Summary: | [CSS Regions] Selecting text inside + outside regions does not work properly | ||
---|---|---|---|
Product: | WebKit | Reporter: | Rebecca Hauck <rhauck> |
Component: | CSS | Assignee: | Nobody <webkit-unassigned> |
Status: | RESOLVED WONTFIX | ||
Severity: | Normal | CC: | alex, bfulgham, buildbot, enrica, hyatt, jfernandez, mihnea, rego, rniwa, WebkitBugTracker, zoltan |
Priority: | P2 | Keywords: | AdobeTracked |
Version: | 528+ (Nightly build) | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
Bug Depends on: | 119622 | ||
Bug Blocks: | 118668 | ||
Attachments: |
Description
Rebecca Hauck
2013-01-14 12:55:01 PST
Created attachment 182613 [details]
test case to reproduce bug
(In reply to comment #0) I tested this a little more after I logged this bug, so this is a revised bug report (I can't edit the original). This bug covers various scenarios of text selection where the selection includes text both inside and outside a region. Each test case has slightly different misbehavior. Steps to reproduce Test 1 - Test 6 are labeled as such in the attached file. Test 1: Selection begins outside the region and ends in the middle of the region, moving from the top of the document downward RESULT: - The selection stops being highlighted as soon as the mouse moves into the region, so the region text never gets highlighted and, depending on how the mouse moves, some of the text outside the region is not highlighted either - window.getSelection().toString() does not contain any of the text outside the region Test 2: Selection begins outside the region, goes through the entire region, and ends outside the region, moving from the top of the document to the bottom of the document RESULT: - The selection highlighting direction reverses when the mouse moves over the region - you briefly see it flip from the start point upward until the mouse moves back out of the region. Once out of the region, everything is highlighted correctly. - window.getSelection().toString() contains all the text that is selected correctly. Test 3: Selection begins inside the region and ends outside the region, moving from the top of the region to the bottom of the document RESULT: - The text inside the region is highlighted correctly, but the highlighting stops when the mouse moves out of the region, so nothing outside of the region is ever highlighted. - window.getSelection().toString() contains all the text that is selected correctly. Test 4: Selection begins inside the region and ends outside the region, moving from the bottom of the region to the top of the document RESULT: - The text inside the region is highlighted correctly, but the highlighting stops when the mouse moves out of the region - window.getSelection().toString() contains nothing Test 5: Selection begins outside the region and ends inside the region, moving from the bottom of the document upward RESULT: Scenario 1: Move the mouse the shortest distance between the 2 points (in test file: straight from word11 to word5) - The selection stops being highlighted as soon as the mouse moves into the region, so the region text never gets highlighted - window.getSelection().toString() contains all the text that is selected correctly. Scenario 2: Move the mouse through all of the text outside of the region before entering the regions (in test file: word11 to word8 then into the region) - Once the mouse enters the region, the selection highlight jumps up to the top of the document or to a random point near the top - I'm unable to stop the selection within the region - window.getSelection().toString() contains all of the text that is highlighted (which is more than I selected with my mouse movement) Test 6: Selection begins outside the region, goes through the entire region, and ends outside the region, moving from the bottom of the document to the top of the document RESULT: - As with Test 5, Scenario 2, once the mouse enters the region, the selection highlight jumps to or near the top of the document - window.getSelection().toString() contains all the text that is selected correctly. Additional Info: I tested all of this in a build which included Zoltan's fix for bug #105641. Lastly, please let me know if this should be split out into separate bugs. I'm currently under the assumption these different symptoms are all from the same underlying problem(s). Created attachment 182662 [details]
Revised test case to reproduce the bug - Use this along with the revised bug report!
Created attachment 183544 [details]
Updated test file
Based on Alan's feedback, updated the test file to put the before-region div before the source div so that the selection steps are always going in DOM order. Also closed some unclosed tags.
Created attachment 206131 [details]
Patch
The patch also reverts the change made in bug #105641, since the Flow Thread compatibility should be performed at editing side, instead of rendering side. Comment on attachment 206131 [details] Patch Attachment 206131 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/985119 New failing tests: fast/regions/autoheight-mixed-nested-complex-regions.html fast/multicol/newmulticol/balance8.html fast/regions/autoheight-abspos-bottom-align.html fast/multicol/newmulticol/balance1.html fast/regions/autoheight-maxheight-simple-break.html fast/multicol/newmulticol/balance3.html fast/regions/autoheight-minmaxheight-mixed-break.html fast/multicol/newmulticol/balance5.html fast/multicol/newmulticol/balance9.html fast/multicol/newmulticol/balance-maxheight1.html fast/multicol/newmulticol/balance4.html fast/multicol/newmulticol/balance-maxheight2.html fast/regions/autoheight-minmaxheight-mixed-break-vrl.html http/tests/inspector/inspect-element.html editing/pasteboard/copy-image-with-alt-text.html fast/multicol/newmulticol/orphans-and-widows-balance.html fast/regions/autoheight-minmaxheight-mixed-break-vlr.html fast/regions/autoheight-minmaxheight-mixed-break-hbt.html fast/regions/autoheight-minmaxheight-simple-nobreak.html fast/regions/autoheight-maxheight-mixed-break.html fast/regions/autoheight-maxheight-simple-nobreak.html editing/selection/select-bidi-run.html fast/multicol/newmulticol/balance7.html fast/multicol/newmulticol/balance2.html fast/regions/autoheight-minmaxheight-simple-break.html Created attachment 206134 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
Comment on attachment 206131 [details] Patch Attachment 206131 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1003011 New failing tests: fast/regions/autoheight-mixed-nested-complex-regions.html fast/multicol/newmulticol/balance8.html fast/regions/autoheight-abspos-bottom-align.html fast/regions/autoheight-mixed-nested-regions.html fast/multicol/newmulticol/balance1.html fast/regions/autoheight-maxheight-simple-break.html fast/multicol/newmulticol/balance3.html fast/regions/autoheight-minmaxheight-mixed-break.html fast/multicol/newmulticol/balance5.html fast/multicol/newmulticol/balance9.html fast/regions/autoheight-mixed-parallel-regions.html fast/multicol/newmulticol/balance-maxheight1.html fast/multicol/newmulticol/balance4.html fast/multicol/newmulticol/balance-maxheight2.html fast/regions/autoheight-minmaxheight-mixed-break-vrl.html http/tests/inspector/inspect-element.html fast/regions/autoheight-maxheight-mixed-break.html fast/multicol/newmulticol/orphans-and-widows-balance.html fast/regions/autoheight-minmaxheight-mixed-break-vlr.html fast/regions/autoheight-minmaxheight-mixed-break-hbt.html fast/regions/autoheight-minmaxheight-simple-nobreak.html editing/text-iterator/basic-iteration.html fast/regions/autoheight-maxheight-simple-nobreak.html fast/multicol/newmulticol/balance7.html fast/regions/autoheight-nested-regions.html fast/loader/javascript-url-in-object.html fast/multicol/newmulticol/balance2.html fast/regions/autoheight-minmaxheight-simple-break.html Created attachment 206138 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.3
Comment on attachment 206131 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206131&action=review > Source/WebCore/ChangeLog:1 > +2013-07-05 Javier Fernandez <jfernandez@igalia.com> This is an informal review, which you may/may not take into account. You need somebody with more editing knowledge to review it. Also, it may be interesting to understand the impact on performance introduced by this code. Also, i see that there are tests that fail and they also fail on my machine after i applied your patch - can you post a patch without the failures? > Source/WebCore/ChangeLog:7 > + I would like to see a description of what this patch is fixing and the approach taken. I do not think it is fixing the whole selection in regions. Maybe we can use 106817 as a master bug and add this patch and other following patches to this master bug? > Source/WebCore/ChangeLog:23 > + at editing side. Can you please explain why? (In reply to comment #12) > (From update of attachment 206131 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=206131&action=review > > > Source/WebCore/ChangeLog:1 > > +2013-07-05 Javier Fernandez <jfernandez@igalia.com> > > This is an informal review, which you may/may not take into account. You need somebody with more editing knowledge to review it. Also, it may be interesting to understand the impact on performance introduced by this code. Also, i see that there are tests that fail and they also fail on my machine after i applied your patch - can you post a patch without the failures? I can confirm such tests failing in my machine as well, my bad. The thing is that the "plainText" function is widely used to the get the Element inner text, for instance, so ew can't touch that function. I've got a patch already that solves the issue (just the issue reported in this bug) without causing any regression, but the approach is to modify the Range::toString instead, in order to skip nodes without renderer and belonging to different Flow Threads. I think modifying the range::toString function is not an option; while not causing any regression, it looks like it follows the Mozilla standards and in such browsers the Range::toString function gets "not-rendering" content as well, so perhaps that's the expected behaviour. I'm exploring now the multi-range selection approach, which, even though being more challenging and ambitious, I think it's the right approach to follow. Also, I agree on the performance issues; I'll focus on them once I've got the valid solution to solve the general problem of Selection with Regions, not just the case reported here. > > > Source/WebCore/ChangeLog:7 > > + > > I would like to see a description of what this patch is fixing and the approach taken. I do not think it is fixing the whole selection in regions. Maybe we can use 106817 as a master bug and add this patch and other following patches to this master bug? Yes, I think the right choice is to select this bug as "meta-bug" for all the selection related issues involving Regions. It's also true that this patch does not solve all the Selection issues when using Regions, but it does it for many of them. I'm starting to realize that we need multi-range or something like that to solve all the issues, but that would require further discussion. I hope to get a preliminary patch soon to start the discussion. > > > Source/WebCore/ChangeLog:23 > > + at editing side. > > Can you please explain why? Well, the patch for 105641 just avoids rendering selections including nodes of different Flow Threads; it doesn't face the nested regions issue specifically, but any other case involving selection of contents from different regions. Besides, the selected range is not correct, so getting the selected content would lead to incorrect results. Fixing the issue at editing side would solve both angles, selection and rendering. Actually, my patch solves both bugs. I came to the conclusion that Selection is a editing matter; it generates Ranges which are used for different purposes, one of them, rendering the selected content. (In reply to comment #13) > (In reply to comment #12) > > (From update of attachment 206131 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=206131&action=review > > > > > Source/WebCore/ChangeLog:1 > > > +2013-07-05 Javier Fernandez <jfernandez@igalia.com> > > > > This is an informal review, which you may/may not take into account. You need somebody with more editing knowledge to review it. Also, it may be interesting to understand the impact on performance introduced by this code. Also, i see that there are tests that fail and they also fail on my machine after i applied your patch - can you post a patch without the failures? > > I can confirm such tests failing in my machine as well, my bad. The thing is that the "plainText" function is widely used to the get the Element inner text, for instance, so ew can't touch that function. > > I've got a patch already that solves the issue (just the issue reported in this bug) without causing any regression, but the approach is to modify the Range::toString instead, in order to skip nodes without renderer and belonging to different Flow Threads. > > I think modifying the range::toString function is not an option; while not causing any regression, it looks like it follows the Mozilla standards and in such browsers the Range::toString function gets "not-rendering" content as well, so perhaps that's the expected behaviour. > > I'm exploring now the multi-range selection approach, which, even though being more challenging and ambitious, I think it's the right approach to follow. > > Also, I agree on the performance issues; I'll focus on them once I've got the valid solution to solve the general problem of Selection with Regions, not just the case reported here. > > > > > > > Source/WebCore/ChangeLog:7 > > > + > > > > I would like to see a description of what this patch is fixing and the approach taken. I do not think it is fixing the whole selection in regions. Maybe we can use 106817 as a master bug and add this patch and other following patches to this master bug? > > Yes, I think the right choice is to select this bug as "meta-bug" for all the selection related issues involving Regions. > > It's also true that this patch does not solve all the Selection issues when using Regions, but it does it for many of them. I'm starting to realize that we need multi-range or something like that to solve all the issues, but that would require further discussion. I hope to get a preliminary patch soon to start the discussion. > Looks like in FF the user can select multiple ranges: https://developer.mozilla.org/en-US/docs/Web/API/Selection > > > > > Source/WebCore/ChangeLog:23 > > > + at editing side. > > > > Can you please explain why? > > Well, the patch for 105641 just avoids rendering selections including nodes of different Flow Threads; it doesn't face the nested regions issue specifically, but any other case involving selection of contents from different regions. > > Besides, the selected range is not correct, so getting the selected content would lead to incorrect results. Fixing the issue at editing side would solve both angles, selection and rendering. Actually, my patch solves both bugs. > > I came to the conclusion that Selection is a editing matter; it generates Ranges which are used for different purposes, one of them, rendering the selected content. Created attachment 206470 [details]
Patch
The change in the Range::toString implies a different behaviour than in FF, so perhaps is not the best approach. I'm studying using the multi-range selection instead.
Comment on attachment 206470 [details] Patch Attachment 206470 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1040098 New failing tests: http/tests/misc/acid3.html fast/dom/Range/range-insertNode-separate-endContainer.html fast/text/international/khmer-selection.html fast/dom/Range/range-expand.html fast/dom/Range/mutation.html fast/dom/Range/range-insertNode-splittext.html editing/selection/select-bidi-run.html Created attachment 206475 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
Comment on attachment 206470 [details] Patch Attachment 206470 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1040111 New failing tests: svg/batik/filters/feTile.svg http/tests/misc/acid3.html fast/dom/Range/range-insertNode-separate-endContainer.html fast/text/international/khmer-selection.html fast/dom/Range/range-expand.html fast/dom/Range/mutation.html fast/dom/Range/range-insertNode-splittext.html editing/selection/select-bidi-run.html Created attachment 206483 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.3
Comment on attachment 206470 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206470&action=review > Source/WebCore/dom/Range.cpp:1070 > + if (!renderer || (startRenderer && (renderer->flowThreadContainingBlock() != startRenderer->flowThreadContainingBlock()))) 1. It might be worth storing the startRenderer->flowThreadContainingBlock on the stack before entering the loop. 2. What if the start node has no renderer? Should we better wait until the first node with a renderer and use that as the reference flow thread? 3. What happens with objects that have display:none? It looks like "!renderer" will change the behavior for that case. > Source/WebCore/editing/FrameSelection.cpp:257 > + if (!atSameFlowThread(base, newExtent)) Should we better patch FrameSelection::setSelection instead? Looks like other functions call that one and might bypass your check. (In reply to comment #20) > (From update of attachment 206470 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=206470&action=review > > > Source/WebCore/dom/Range.cpp:1070 > > + if (!renderer || (startRenderer && (renderer->flowThreadContainingBlock() != startRenderer->flowThreadContainingBlock()))) > > 1. It might be worth storing the startRenderer->flowThreadContainingBlock on the stack before entering the loop. > 2. What if the start node has no renderer? Should we better wait until the first node with a renderer and use that as the reference flow thread? > 3. What happens with objects that have display:none? It looks like "!renderer" will change the behavior for that case. Yes, I'm aware of those issues and next version of the patch won't touch the Range class, as it's causing several regressions. The current approach would be based on multi-range selection, which, even more complex and challenging, I think it's the right approach. > > > Source/WebCore/editing/FrameSelection.cpp:257 > > + if (!atSameFlowThread(base, newExtent)) > > Should we better patch FrameSelection::setSelection instead? Looks like other functions call that one and might bypass your check. Umm, yes, it seems a better place to put the check, indeed. But I have to check out the fields modified in setNonDirectionalSelectionIfNeeded, since perhaps they should be kept untouched. Created attachment 207284 [details]
Patch
Pay special attention to the splitRegions call in the validate() method, since it has an important impact on performance; there should be other ways to do it without having to inspect the DOM everytime the selection changes.
Comment on attachment 207284 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207284&action=review > Source/WebCore/editing/VisibleSelection.h:95 > + PassRefPtr<Range> getRangeAt(int) const; > + int rangeCount() const; We don't want to support these APIs. r-. (In reply to comment #23) > (From update of attachment 207284 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207284&action=review > > > Source/WebCore/editing/VisibleSelection.h:95 > > + PassRefPtr<Range> getRangeAt(int) const; > > + int rangeCount() const; > > We don't want to support these APIs. r-. I have some questions. Is it your opinion or Apple's point of view on this decision? Can you share some details what was behind this decision? Are you planning to support multi range selection ever? If yes, is there any internal solution already for the problem, and you want to merge that code later to the trunk? I'm asking because there is a comment here: http://trac.webkit.org/browser/trunk/Source/WebCore/editing/VisibleSelection.h#L92 about multi-range selection which suggest that not supporting it is just a temporary state. If we don't want to support multi-range selection, then we can agree on the correctness of the solution which prevents selection over different flow threads. Or do you have another idea? Javier started a thread on the mailing list regarding to this topic too. (https://lists.webkit.org/pipermail/webkit-dev/2013-July/025179.html) It would be good to hear your opinion! Thanks. Created attachment 229894 [details]
Patch (WIP)
Comment on attachment 229894 [details] Patch (WIP) This is just a work-in-progress alternative solution for selection. The actual solution, for the time being just fulfilling the Editing/Selection specs, was already implemented in patch attached to bug 131511. This approach breaks DOM Selection nature and implements a RenderTree based algorithm. If you are interested, you can find additional information on these solutions for Selection here: https://github.com/Igalia/css-regions-selection/wiki/Selection-&-CSS-Regions CSS Regions were removed in Bug 174978. |