Bug 119137

Created attachment 207515 [details] HTML test to reproduce the issue

Created attachment 207631 [details] Patch

Created attachment 207632 [details] Patch

Created attachment 207639 [details] Patch

Comment on attachment 207639 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207639&action=review > Source/WebCore/ChangeLog:1 > +2013-07-29 Javier Fernandez <jfernandez@igalia.com> > LayoutTests/ChangeLog:1 > +2013-07-29 Manuel Rego Casasnovas <rego@igalia.com> The author is different for the 2 changelogs. Is it intentional? > Source/WebCore/rendering/RenderView.cpp:-728 > - if ((start && end) && (start->flowThreadContainingBlock() != end->flowThreadContainingBlock())) I think at the point we remove this condition we should be able to select content through different regions. I have some questions. Wouldn't be easier to fix the highlighting of the selection? Or do you already have a plan to achieve that based on the actual patch? (Is this change necessary beside with the work in bug #106817 ?)

(In reply to comment #5) > (From update of attachment 207639 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207639&action=review > > > Source/WebCore/ChangeLog:1 > > +2013-07-29 Javier Fernandez <jfernandez@igalia.com> > > LayoutTests/ChangeLog:1 > > +2013-07-29 Manuel Rego Casasnovas <rego@igalia.com> > > The author is different for the 2 changelogs. Is it intentional? > The WebCore patch was extracted, actually, from the patch posted at bug #106817, but, since it covers only a specific case, a new and simpler test was added. I guess that's the reason, but rego should tell. > > Source/WebCore/rendering/RenderView.cpp:-728 > > - if ((start && end) && (start->flowThreadContainingBlock() != end->flowThreadContainingBlock())) > > I think at the point we remove this condition we should be able to select content through different regions. Actually, this patch makes the highlighting to behave exactly as yours, but it solved at editing side, which I think is more appropriated and solves also other issues related to the selected content. More on this below. > > I have some questions. > > Wouldn't be easier to fix the highlighting of the selection? Or do you already have a plan to achieve that based on the actual patch? (Is this change necessary beside with the work in bug #106817 ?) As I said before, this patch solves the highlighting issue as well. The approach followed in the patch for bug #106817 is based on multi-range, which was strongly rejected by some reviewers. Actually, there is an ongoing thread about this matter in the mailing list, still without many contributions, though. With this patch, when user selects content starting outside the region and ending inside, not only the highlighting works, but also the content inside the region is skipped too, so we think is more coherent which what user would expect. It's important to notice that both, this patch and the one you provided for bug #105641 assume we won't allow the user to select content from different Flow Threads, which we thought it was a good start. However, given the negative feedback we have been getting from the multi-range approach, we are thinking on keeping the selection as DOM based, even when the user would get weird selected content when flow named nodes are inside the selection boundaries.

Created attachment 207874 [details] additional test case (In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 207639 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=207639&action=review > > > > > Source/WebCore/ChangeLog:1 > > > +2013-07-29 Javier Fernandez <jfernandez@igalia.com> > > > LayoutTests/ChangeLog:1 > > > +2013-07-29 Manuel Rego Casasnovas <rego@igalia.com> > > > > The author is different for the 2 changelogs. Is it intentional? > > > > The WebCore patch was extracted, actually, from the patch posted at bug #106817, but, since it covers only a specific case, a new and simpler test was added. I guess that's the reason, but rego should tell. > > > > Source/WebCore/rendering/RenderView.cpp:-728 > > > - if ((start && end) && (start->flowThreadContainingBlock() != end->flowThreadContainingBlock())) > > > > I think at the point we remove this condition we should be able to select content through different regions. > > Actually, this patch makes the highlighting to behave exactly as yours, but it solved at editing side, which I think is more appropriated and solves also other issues related to the selected content. More on this below. Yes, that's true! > > I have some questions. > > > > Wouldn't be easier to fix the highlighting of the selection? Or do you already have a plan to achieve that based on the actual patch? (Is this change necessary beside with the work in bug #106817 ?) > > As I said before, this patch solves the highlighting issue as well. The approach followed in the patch for bug #106817 is based on multi-range, which was strongly rejected by some reviewers. Actually, there is an ongoing thread about this matter in the mailing list, still without many contributions, though. > > With this patch, when user selects content starting outside the region and ending inside, not only the highlighting works, but also the content inside the region is skipped too, so we think is more coherent which what user would expect. > > It's important to notice that both, this patch and the one you provided for bug #105641 assume we won't allow the user to select content from different Flow Threads, which we thought it was a good start. However, given the negative feedback we have been getting from the multi-range approach, we are thinking on keeping the selection as DOM based, even when the user would get weird selected content when flow named nodes are inside the selection boundaries. Thanks for the clarification. I ramped up on the topic now. We should go along with this approach for now. I like the change, but I miss some other cases from the solution. I attached a test case which includes some bad smell. As you see the highlighted content and the actual selected content isn't going to be the same. Can you take a look at it? In the meantime I'm going to raise some questions in the previous bug to Ryosuke.

(In reply to comment #7) > Created an attachment (id=207874) [details] > additional test case > > (In reply to comment #6) > > (In reply to comment #5) > > > (From update of attachment 207639 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=207639&action=review > > > > > > > Source/WebCore/ChangeLog:1 > > > > +2013-07-29 Javier Fernandez <jfernandez@igalia.com> > > > > LayoutTests/ChangeLog:1 > > > > +2013-07-29 Manuel Rego Casasnovas <rego@igalia.com> > > > > > > The author is different for the 2 changelogs. Is it intentional? > > > > > > > The WebCore patch was extracted, actually, from the patch posted at bug #106817, but, since it covers only a specific case, a new and simpler test was added. I guess that's the reason, but rego should tell. About the author it was just basically because of Javi did the patch itself and I wrote the test. But we can change it if you think it's wrong. > Thanks for the clarification. I ramped up on the topic now. We should go along with this approach for now. I like the change, but I miss some other cases from the solution. > I attached a test case which includes some bad smell. As you see the highlighted content and the actual selected content isn't going to be the same. Can you take a look at it? > In the meantime I'm going to raise some questions in the previous bug to Ryosuke. The idea of this patch was just to fix the initial issue. As stopping the highlight at render side don't seem like the best option compared to doing the same at editing side. We know that there're still more use cases pending to be fixed. Some of them (like the one you attached) are fixed with the patch for bug #106817 (and other more complex will appear under the meta bug #118668). However, we preferred to split this in order to make easier the review process. So, this is a first simple patch to fix some concrete issues of doing this in the render side instead of editing side. Maybe we could get this initial patch landed and carry on fixing other stuff in later bugs.

(In reply to comment #8) > About the author it was just basically because of Javi did the patch itself and I wrote the test. But we can change it if you think it's wrong. That's fine, but it was just odd. :) > The idea of this patch was just to fix the initial issue. As stopping the highlight at render side don't seem like the best option compared to doing the same at editing side. I agree, I support the solution! > We know that there're still more use cases pending to be fixed. Some of them (like the one you attached) are fixed with the patch for bug #106817 (and other more complex will appear under the meta bug #118668). However, we preferred to split this in order to make easier the review process. > > So, this is a first simple patch to fix some concrete issues of doing this in the render side instead of editing side. Maybe we could get this initial patch landed and carry on fixing other stuff in later bugs. I would wait for response from Ryosuke on the previous bug, but since this change doesn't harm anything and fixes an issue, I'm saying let's do this.

Comment on attachment 207639 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207639&action=review > Source/WebCore/editing/FrameSelection.cpp:307 > + if (atDifferentFlowThread(s.base(), s.extent())) > + return; > + This is not the right place to fix this. We need to do this inside VisibleSelection::validate probably right before or right after we call adjustSelectionToAvoidCrossingShadowBoundaries.

(In reply to comment #10) > (From update of attachment 207639 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207639&action=review > > > Source/WebCore/editing/FrameSelection.cpp:307 > > + if (atDifferentFlowThread(s.base(), s.extent())) > > + return; > > + > > This is not the right place to fix this. We need to do this inside VisibleSelection::validate probably right before or right after we call adjustSelectionToAvoidCrossingShadowBoundaries. The purpose of this patch is to avoid setting a new VisibleSelection instance to the FrameSelection if it's incompatible in terms of RenderFlow. For the shake of clarity, I've taken the assumption that only selections starting and ending in the same FlowThread, or both outside, are allowed. On the other hand, that's the current behaviour after the fix for the bug #105641. This patch keep that behaviour form the rendering point of view, but at the same time targeting also the issues related to the selected content. Getting back to your suggestion of implementing this in the VisibleSelection::validate(), this method is intended for adjusting the new Selection to fit in some validation rules. It's not necessary to adjust the new selection, since the one already set before in the FrameSelection instance is correct. If done at validation() we should move back the extent Position to the last one FlowThread compatible, which it would require to explore the DOM. This could have also an impact on performance, even more considering validate(9 is called a lot of times, overall, when selection is performed by user gestures. After all this, do you still think VisibleSelection::validate() is the right place to put this logic ?

Comment on attachment 207639 [details] Patch Clearing flags.

*** This bug has been marked as a duplicate of bug 131511 ***