Bug 45381

Created attachment 67807 [details] Patch proposal Attaching a patch that would fix the issue. It adds some -pretty simple yet useful- new API to WebCore::RenderListMarker and WebCore::RenderListItem to provide a way to retrieve the actual full text involved in a list item, that is, the item marker (like "1", from "1. This is an item") and the item marker's suffix (like ". " in "1. This is an item"). So far, it was possible to retrieve the marker but not the suffix, which was dinamically calculated and drawn in the paint() method of WebCore::RenderListMarker. However, as we need the suffix to be as much as accurate as possible when exposing list items to AT's through the ATK implementation (GTK), we need a way to get that suffix as well, and I thought of this one as the best way to do it. The other part of the patch is just in AccessibilityObjectWrapperAtk.cpp (GTK port) and fixes some problems considering list markers in the AtkText interface implementation, now doing it in a more consistent way, IMHO. Hence, now asking for review.

Attachment 67807 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1209: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 67808 [details] Patch proposal Damm it... always remember to run the check-webkit-style script after attaching the patch. Sorry! This is the patch to review then :-P

Comment on attachment 67808 [details] Patch proposal View in context: https://bugs.webkit.org/attachment.cgi?id=67808&action=review i don't know how other people feel about adding a variable to RenderListMarker. sometimes they are quite picky about that. need a layout test for this however > WebCore/ChangeLog:39 > + indent comments when they are underneath the a filename. it makes it easier to read > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1218 > + RenderObject* renderer = static_cast<const AccessibilityRenderObject*>(object)->renderer(); check if renderer is nil as well > WebCore/rendering/RenderListItem.cpp:325 > + const String& markerTxt = m_marker->text(); var name should be markerText

(In reply to comment #4) > (From update of attachment 67808 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=67808&action=review > > i don't know how other people feel about adding a variable to RenderListMarker. sometimes they are quite picky about that. Well, that's the best way I found to allow returning the marker's suffix without having to recalculate it each time. But if there's a better way to do it, I'm more than happy to change it. > need a layout test for this however Wouldn't it be enough with an unit test to check the marker is what it's supposed to be? > > WebCore/ChangeLog:39 > > + > > indent comments when they are underneath the a filename. it makes it easier to read Ok > > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1218 > > + RenderObject* renderer = static_cast<const AccessibilityRenderObject*>(object)->renderer(); > > check if renderer is nil as well Ok. > > WebCore/rendering/RenderListItem.cpp:325 > > + const String& markerTxt = m_marker->text(); > > var name should be markerText Ok.

Created attachment 68509 [details] Patch proposal + unit tes updated (In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 67808 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=67808&action=review > > > > i don't know how other people feel about adding a variable to RenderListMarker. sometimes they are quite picky about that. > > Well, that's the best way I found to allow returning the marker's suffix without having to recalculate it each time. But if there's a better way to do it, I'm more than happy to change it. I finally found a simple way to expose the marker's suffix through a public function without having to add that variable to RenderListMarker, and it was just delegating on the internal static function listMarkerSuffix(), and then doing some small changes in the new funcion RenderListItem::markerTextWithSuffix(). And to be honest, I recognize I like it more this way, which hast the plus of not relying in the painting process to get the right value of the marker suffix. > > need a layout test for this however > > Wouldn't it be enough with an unit test to check the marker is what it's supposed to be? I updated the unit test for ATK to ensure that the exposition of the marker is the right one. Guess this should be enough, but you (Chris) will better tell > > > WebCore/ChangeLog:39 > > > + > > > > indent comments when they are underneath the a filename. it makes it easier to read > > Ok Done. > > > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:1218 > > > + RenderObject* renderer = static_cast<const AccessibilityRenderObject*>(object)->renderer(); > > > > check if renderer is nil as well > > Ok. Done. > > > WebCore/rendering/RenderListItem.cpp:325 > > > + const String& markerTxt = m_marker->text(); > > > > var name should be markerText > > Ok. Done.

Comment on attachment 68509 [details] Patch proposal + unit tes updated when you get the text of an item with a list marker, should there be a space between the suffix and the actual text? atk_text_get_text(ATK_TEXT(item3), 0, -1), ==, "3.text and a link" otherwise this patch is looking pretty good

(In reply to comment #7) > (From update of attachment 68509 [details]) > when you get the text of an item with a list marker, should there be a space between the suffix and the actual text? > atk_text_get_text(ATK_TEXT(item3), 0, -1), ==, "3.text and a link" > > otherwise this patch is looking pretty good Yeah, actually the previous version of the patch would need that extra space in the test, as I was also returning the space that sometimes comes along with the suffix in the RenderListMarker... But then I thought again when redoing the patch without using the suffix, and I realized that the aditional space that sometimes is added to the suffix seemed to be too dependant on the painting process (where it's decided whether to put or not that extra space), and so to return that space along with the suffix would be too much hassle, would complicate this patch too much just because of it and, after all, if I wanted to be coherent, a function RenderListMarker::suffix() should return just the suffix, not the suffix and that -sometimes used while painting- extra space. On top of that, from the ATK world POV, I don't think this is a big deal anyway, because at least a 1-character suffix will always be there separating the marker and the item's content (if it's not a character like '.', it will be at least ' '), so I don't think it was worth messing with that kind of things. Of course, I can be wrong... but I honestly think this is a better, cleaner and good enough implementation. But in case I was wrong, just point it out and I'd look for a reliable way to put that extra space when needed, just the same way it would be done during the execution of the paint() method.

(In reply to comment #8) > (In reply to comment #7) > [...] > Of course, I can be wrong... but I honestly think this is a better, cleaner and good enough > implementation. But in case I was wrong, just point it out and I'd look for a reliable way to > put that extra space when needed, just the same way it would be done during the execution > of the paint() method. I forgot to explicitly say that before, but I'd like to hear as well Joanie's opinion on this topic, as she probably knows better what it would be acceptable/the right thing to do in this case. So wrapping up, Joanmarie... would it be ok to expose a list item like "1.item content here" or would it be needed to do it like "1. item content here", instead?

(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > [...] > > Of course, I can be wrong... but I honestly think this is a better, cleaner and good enough > > implementation. But in case I was wrong, just point it out and I'd look for a reliable way to > > put that extra space when needed, just the same way it would be done during the execution > > of the paint() method. > > I forgot to explicitly say that before, but I'd like to hear as well Joanie's opinion on this topic, as she probably knows better what it would be acceptable/the right thing to do in this case. > > So wrapping up, Joanmarie... would it be ok to expose a list item like "1.item content here" or would it be needed to do it like "1. item content here", instead? Well, the goal is (of course) to present to the user what's on the screen. And what's visually on the screen looks like "1. item content here". The fact that this appearance was achieved by rendering magic other than the addition of an actual space character is, as far as the end user is concerned, irrelevant. Thus I'm afraid we do need the space added.

(In reply to comment #10) > [...] > Well, the goal is (of course) to present to the user what's on the screen. And what's visually > on the screen looks like "1. item content here". The fact that this appearance was achieved > by rendering magic other than the addition of an actual space character is, as far as the end > user is concerned, irrelevant. Thus I'm afraid we do need the space added. Ok. Then I'll try to come up with a patch dealing with that extra space. I've just checked what firefox does in thit case and it seems they also add that extra space, so let's follow the ff lead. Thanks for the feedback.

Created attachment 68677 [details] Patch proposal + unit test updated (In reply to comment #11) > (In reply to comment #10) > > [...] > > Well, the goal is (of course) to present to the user what's on the screen. And what's visually > > on the screen looks like "1. item content here". The fact that this appearance was achieved > > by rendering magic other than the addition of an actual space character is, as far as the end > > user is concerned, irrelevant. Thus I'm afraid we do need the space added. > > Ok. Then I'll try to come up with a patch dealing with that extra space. I've just checked what firefox does in thit case and it seems they also add that extra space, so let's follow the ff lead. > > Thanks for the feedback. Attaching a new patch that also returns the extra space, when needed, along with the marker's suffix, so calling to RenderListMarker::suffix() returns the same string that is going to be rendered when painting between the list item and the item's content. Hence, asking for review over this new patch.

Comment on attachment 68677 [details] Patch proposal + unit test updated looks ok. thanks r=me

(In reply to comment #13) > (From update of attachment 68677 [details]) > looks ok. thanks > r=me Thanks for the review. Could you (or some other reviewer/committer) set the cq+ flag? I forgot to set the cq? in advance. Thanks!

Comment on attachment 68677 [details] Patch proposal + unit test updated Rejecting patch 68677 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 68677]" exit_code: 2 Cleaning working directory Updating working directory Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2 Full output: http://queues.webkit.org/results/4002175

(In reply to comment #15) > (From update of attachment 68677 [details]) > Rejecting patch 68677 from commit-queue. > > Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 68677]" exit_code: 2 > Cleaning working directory > Updating working directory > Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2 > > Full output: http://queues.webkit.org/results/4002175 I don't know what could have happened here, since the patch applies in my local machine perfectly on top of the very last commit. Also it looks this issue also happened to other recent patches in the commit queue, so perhaps this is not an isolated issues, but something more global. See bugs 46621, 46538 and 46545, for instance, at the commit queue: http://webkit-commit-queue.appspot.com/queue-status/commit-queue If there's anything I can do from my side, don't hesitate to let me know.

Comment on attachment 68677 [details] Patch proposal + unit test updated Rejecting patch 68677 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2 Last 500 characters of output: k All tests successful. Files=14, Tests=304, 2 wallclock secs ( 1.13 cusr + 0.23 csys = 1.36 CPU) Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/abarth/git/webkit-queue/LayoutTests Testing 21379 test cases. media/invalid-media-url-crash.html -> crashed Exiting early after 1 failures. 17591 tests run. 597.37s total testing time 17590 test cases (99%) succeeded 1 test case (<1%) crashed 29 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/4058163

Comment on attachment 68677 [details] Patch proposal + unit test updated Looks like a flaky test.

Comment on attachment 68677 [details] Patch proposal + unit test updated Clearing flags on attachment: 68677 Committed r68415: <http://trac.webkit.org/changeset/68415>

All reviewed patches have been landed. Closing bug.

http://trac.webkit.org/changeset/68415 might have broken Leopard Intel Debug (Tests) The following changes are on the blame list: http://trac.webkit.org/changeset/68416 http://trac.webkit.org/changeset/68417 http://trac.webkit.org/changeset/68415