[CSS Regions] Selection highlight doesn't match DOM selection

• Component: CSS
• Status: RESOLVED FIXED

[Manuel Rego Casasnovas]

Selection in CSS Regions does not always highlight the same content that is actually selected in the DOM (and returned when you copy&paste).

The main issue is that "content nodes" (the ones with -webkit-flow-into) property are moved to a special subtrees in the Render Tree. Where they appear as children of RenderFlowThread elements.

Selection highlight is based on the Render Tree, it traverse the Render Objects from start to end marking the elements as selected. There're several problems when you mix this with the RenderFlowThreads:
* You can start the selection in the RenderFlowThrea subtree and finish in the regular subtree. And the selection algorithm might not reach the end from the start (this issue was fixed in r155058 by bug #119622 with a workaround).
* The selection doesn't process the elements that have been moved to the RenderFlowThrea subtree even when in the DOM they were actually between start and end.

These problems cause that selected content and highlight does not always match.

[Manuel Rego Casasnovas]

Created attachment 229086 [details]
Patch

[WebKit Commit Bot]

Attachment 229086 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderView.h:292:  The parameter name "end" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Manuel Rego Casasnovas]

I've just uploaded a patch to fix these problems using the following approach:
* We divide the Render tree in subtrees (one subtree will be the RenderView and then one subtree per RenderFlowThread).
* For each subtree we calculate the start and end positions according to the DOM selection.
* Then we call the selection methods in RenderView for each subtree.

You can find more detailed information about this approach at: https://github.com/Igalia/css-regions-selection/wiki/Selection-&-CSS-Regions#subtrees-approach

[Manuel Rego Casasnovas]

Created attachment 229113 [details]
Patch

Trying to fix Windows build.

[WebKit Commit Bot]

Attachment 229113 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderView.h:292:  The parameter name "end" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andrei Bucur]

Comment on attachment 229113 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=229113&action=review

Really happy to see this work moving forward. This is just an informal review as this is not my expertise area.

> Source/WebCore/ChangeLog:11
> +        then for each RenderFlowThread (which are siblings of RenderView) we will have an extra subtree.

Children, not siblings.

> Source/WebCore/rendering/RenderObject.cpp:1545
> +    RenderFlowThread* flowThread = flowThreadContainingBlock();

Maybe we can create a selectionRoot() function on RenderObject that could abstract this. Probably there will be other type of roots in the future (flexbox maybe?).

> Source/WebCore/rendering/RenderView.cpp:664
> +    for (RenderObject* child = firstChild(); child; child = child->nextSibling()) {

This loop should be moved inside FlowThreadController. There's a list of all the named flow threads. We'll also get better regions code encapsulation.

> Source/WebCore/rendering/RenderView.cpp:717
> +    for (RenderObject* child = firstChild(); child; child = child->nextSibling()) {

Same as above. Move this inside FlowThreadController.

> Source/WebCore/rendering/RenderView.cpp:784
> +    for (RenderObject* child = firstChild(); child; child = child->nextSibling()) {

Move this to FlowThreadController.

> Source/WebCore/rendering/RenderView.cpp:795
> +        RefPtr<Range> initialRange = Range::create(document(), start->node(), startPos, end->node(), endPos);

Maybe you can put the code below inside a helper function with a suggestive name :). Something like splitSelectionBetweenSubtreeRoots() or setSelectionInsideSubtreeRoots() that's called if there's more than one subtree (if you have named flows).

> Source/WebCore/rendering/RenderView.cpp:806
> +            SelectionSubtreeRoot* flowThread = renderer->flowThreadContainingBlock();

You could use renderer->selectionRoot() here as I suggested above.

> Source/WebCore/rendering/RenderView.cpp:-816
> -    m_selectionStart = start;

Shouldn't we also delete these fields from RenderView.h? They are encapsulated inside the root object.

[Manuel Rego Casasnovas]

(In reply to comment #6)
> (From update of attachment 229113 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229113&action=review
> 
> Really happy to see this work moving forward. This is just an informal review as this is not my expertise area.

Thanks for the quick review.

> > Source/WebCore/ChangeLog:11
> > +        then for each RenderFlowThread (which are siblings of RenderView) we will have an extra subtree.
> 
> Children, not siblings.

Yep.

> > Source/WebCore/rendering/RenderObject.cpp:1545
> > +    RenderFlowThread* flowThread = flowThreadContainingBlock();
> 
> Maybe we can create a selectionRoot() function on RenderObject that could abstract this. Probably there will be other type of roots in the future (flexbox maybe?).

I have created it, as it makes the code simpler and easier to understand.

As a side note, I'm not sure if it'll be needed for other cases, as regions is the only one breaking the order of the DOM tree in the Render tree so far. More info at:
https://github.com/Igalia/css-regions-selection/wiki/Selection-&-Other-layout-models

> > Source/WebCore/rendering/RenderView.cpp:664
> > +    for (RenderObject* child = firstChild(); child; child = child->nextSibling()) {
> 
> This loop should be moved inside FlowThreadController. There's a list of all the named flow threads. We'll also get better regions code encapsulation.

Great, didn't realize about FlowThreadController.

> > Source/WebCore/rendering/RenderView.cpp:795
> > +        RefPtr<Range> initialRange = Range::create(document(), start->node(), startPos, end->node(), endPos);
> 
> Maybe you can put the code below inside a helper function with a suggestive name :). Something like splitSelectionBetweenSubtreeRoots() or setSelectionInsideSubtreeRoots() that's called if there's more than one subtree (if you have named flows).

Nice suggestion, I've moved it to a new method.

> > Source/WebCore/rendering/RenderView.cpp:806
> > +            SelectionSubtreeRoot* flowThread = renderer->flowThreadContainingBlock();
> 
> You could use renderer->selectionRoot() here as I suggested above.

Done.

> > Source/WebCore/rendering/RenderView.cpp:-816
> > -    m_selectionStart = start;
> 
> Shouldn't we also delete these fields from RenderView.h? They are encapsulated inside the root object.

We still need these fields to store the original positions from the user's selection.
If he only selects inside the RenderView subtree they'll have the very same value than SelectionSubtreeRoot object.
However, if the selection is from the RenderView to a RenderFlowThread, or only inside the RenderFlowThread, we still need to know the original selection done by the user or we'll need to do a new loop over the DOM tree to know the start and end positions.

[Manuel Rego Casasnovas]

Created attachment 229126 [details]
Patch

[WebKit Commit Bot]

Attachment 229126 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderView.h:292:  The parameter name "end" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/rendering/RenderView.h:293:  The parameter name "end" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dave Hyatt]

Comment on attachment 229126 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=229126&action=review

This looks good. I just have two issues:

(1) I think you want RenderNamedFlowThread to inherit from SelectionSubtreeRoot, since I don't think in-flow RenderFlowThreads (e.g., the new multi-column code) need to have a separate selection.

(2) Since selectionRoot() can never be null, make sure to use a & rather than a * everywhere.

Other than that, the approach looks fine.

> Source/WebCore/rendering/RenderFlowThread.h:60
> +class RenderFlowThread: public RenderBlockFlow, public SelectionSubtreeRoot {

I don't think all flow threads should be selection subtree roots. I think only named RenderFlowThreads should be selection subtree roots. In-flow flow threads have no reason to maintain a separate selection subtree.

> Source/WebCore/rendering/RenderObject.cpp:1550
> +SelectionSubtreeRoot* RenderObject::selectionRoot() const
> +{
> +    RenderFlowThread* flowThread = flowThreadContainingBlock();
> +    if (flowThread)
> +        return flowThread;
> +
> +    return &view();
> +}

This is going to actually cause in-flow RenderFlowThreads to be selection roots. I don't think we want this, since a single subtree is capable of crossing into and out of in-flow RenderFlowThreads. You should limit this to only out-of-flow RenderFlowThreads (i.e., named RenderFlowThreads).

Also, the selectionRoot() can never be null, so I think this should return a reference rather than a pointer.

> Source/WebCore/rendering/RenderView.cpp:795
> +    // Initializate map for RenderView and all RenderFlowThreads

Typo: "Initialize"

> Source/WebCore/rendering/RenderView.cpp:857
> +void RenderView::setSubtreeSelection(SelectionSubtreeRoot* root, RenderObject* start, int startPos, RenderObject* end, int endPos, SelectionRepaintMode blockRepaintMode)
> +{

SelectionSubtreeRoot& would be better (since it can't be null).

[Manuel Rego Casasnovas]

(In reply to comment #10)
> (From update of attachment 229126 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=229126&action=review
> 
> This looks good. I just have two issues:

Thanks for the review.

> (1) I think you want RenderNamedFlowThread to inherit from SelectionSubtreeRoot, since I don't think in-flow RenderFlowThreads (e.g., the new multi-column code) need to have a separate selection.

Yes, you're right. I didn't know about the in-flow RenderFlowThreads in the multi-column code, but it's clear that we don't need a separate selection for this.

> (2) Since selectionRoot() can never be null, make sure to use a & rather than a * everywhere.

Yes, sure.

> > Source/WebCore/rendering/RenderFlowThread.h:60
> > +class RenderFlowThread: public RenderBlockFlow, public SelectionSubtreeRoot {
> 
> I don't think all flow threads should be selection subtree roots. I think only named RenderFlowThreads should be selection subtree roots. In-flow flow threads have no reason to maintain a separate selection subtree.

I've changed it to RenderNamedFlowThread.

> > Source/WebCore/rendering/RenderObject.cpp:1550
> > +SelectionSubtreeRoot* RenderObject::selectionRoot() const
> > +{
> > +    RenderFlowThread* flowThread = flowThreadContainingBlock();
> > +    if (flowThread)
> > +        return flowThread;
> > +
> > +    return &view();
> > +}
> 
> This is going to actually cause in-flow RenderFlowThreads to be selection roots. I don't think we want this, since a single subtree is capable of crossing into and out of in-flow RenderFlowThreads. You should limit this to only out-of-flow RenderFlowThreads (i.e., named RenderFlowThreads).

I've changed it to identify only RenderNamedFlowThreads or RenderView as selection root.

> Also, the selectionRoot() can never be null, so I think this should return a reference rather than a pointer.

Done.

> > Source/WebCore/rendering/RenderView.cpp:795
> > +    // Initializate map for RenderView and all RenderFlowThreads
> 
> Typo: "Initialize"

Fixed.

> > Source/WebCore/rendering/RenderView.cpp:857
> > +void RenderView::setSubtreeSelection(SelectionSubtreeRoot* root, RenderObject* start, int startPos, RenderObject* end, int endPos, SelectionRepaintMode blockRepaintMode)
> > +{
> 
> SelectionSubtreeRoot& would be better (since it can't be null).

Changed this and other similar methods.

I'm uploading a new patch applying all the suggested changes.

[Manuel Rego Casasnovas]

Created attachment 229575 [details]
Patch

[WebKit Commit Bot]

Attachment 229575 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderView.h:298:  The parameter name "end" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/rendering/RenderView.h:299:  The parameter name "end" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dave Hyatt]

Comment on attachment 229575 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 229575 [details]
Patch

Clearing flags on attachment: 229575

Committed r167652: <http://trac.webkit.org/changeset/167652>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This change made fast/regions/cssom/region-range-for-box-crash.html assert every time on debug bots, bug 131982.

[Dave Hyatt]

This change has caused an assertion in repaintSelection with the new multicolumn code.

[Dave Hyatt]

I filed 

https://bugs.webkit.org/show_bug.cgi?id=132019

to cover the crashers. Patch included.

[Manuel Rego Casasnovas]

(In reply to comment #17)
> This change made fast/regions/cssom/region-range-for-box-crash.html assert every time on debug bots, bug 131982.

The patch has landed in r167675.

[Manuel Rego Casasnovas]

*** Bug 119137 has been marked as a duplicate of this bug. ***