Bug 151380 – Selection should work across shadow boundary when initiated by a mouse drag

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

User should be able to select contents across a shadow boundary.

[Radar WebKit Bug Importer]

<rdar://problem/24363872>

[Ryosuke Niwa]

Created attachment 350744 [details]
WIP

[EWS Watchlist]

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


ERROR: Source/WebCore/editing/VisibleSelection.cpp:533:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 1 files


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

[Ryosuke Niwa]

Created attachment 350851 [details]
Adds new behavior

[Antti Koivisto]

Comment on attachment 350851 [details]
Adds new behavior

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

> Source/WebCore/editing/VisibleSelection.cpp:525
> +/*
> +    for (RefPtr<Element> host = shadowRoot->host(); host; ) {
> +        if (host->hasEditableStyle())
> +            return true;
> +        auto* nextRoot = host->containingShadowRoot();
> +        if (!nextRoot)
> +            break;
> +        host = nextRoot->host();
> +    }
> +*/

Why is this code commented out?

[Ryosuke Niwa]

(In reply to Antti Koivisto from comment #5)
> Comment on attachment 350851 [details]
> Adds new behavior
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350851&action=review
> 
> > Source/WebCore/editing/VisibleSelection.cpp:525
> > +/*
> > +    for (RefPtr<Element> host = shadowRoot->host(); host; ) {
> > +        if (host->hasEditableStyle())
> > +            return true;
> > +        auto* nextRoot = host->containingShadowRoot();
> > +        if (!nextRoot)
> > +            break;
> > +        host = nextRoot->host();
> > +    }
> > +*/
> 
> Why is this code commented out?

Oh oops, that code was there to make sure a test catches a bug. This commented out code is wrong in some edge case, and needs to be removed. Sorry, forgot to remove it :(

[Antti Koivisto]

Comment on attachment 350851 [details]
Adds new behavior

Ok, r=me without the commented out part.

[Ryosuke Niwa]

I should actually turn this off by default since supporting selecting across shadow boundaries is useless until copy & paste or JS API support is added.

[Ryosuke Niwa]

Thanks for the review! Would you still stand by your r=me if I disabled the feature by default??

[Antti Koivisto]

Sure!

[Ryosuke Niwa]

Created attachment 350881 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 350881 [details]
Patch for landing

Wait for EWS.

[Ryosuke Niwa]

Created attachment 350882 [details]
Removed the dead code

[Ryosuke Niwa]

Comment on attachment 350882 [details]
Removed the dead code

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

> Source/WebCore/editing/VisibleSelection.cpp:541
> -        m_extent = adjustPositionForEnd(m_end, m_start.containerNode());
> +        m_extent = adjustPositionForEnd(m_end, startNode.ptr());

Hm... on my second thought, this change is probably not right. containerNode of a position isn't always anchorNode.

> Source/WebCore/editing/VisibleSelection.cpp:544
> -        m_extent = adjustPositionForStart(m_start, m_end.containerNode());
> +        m_extent = adjustPositionForStart(m_start, endNode.ptr());

Ditto.

[Wenson Hsieh]

Comment on attachment 350882 [details]
Removed the dead code

r=me (and antti)

[Ryosuke Niwa]

Committed r236519: <https://trac.webkit.org/changeset/236519>

[Ryosuke Niwa]

(In reply to Ryosuke Niwa from comment #14)
> Comment on attachment 350882 [details]
> Removed the dead code
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350882&action=review
> 
> > Source/WebCore/editing/VisibleSelection.cpp:541
> > -        m_extent = adjustPositionForEnd(m_end, m_start.containerNode());
> > +        m_extent = adjustPositionForEnd(m_end, startNode.ptr());
> 
> Hm... on my second thought, this change is probably not right. containerNode
> of a position isn't always anchorNode.
> 
> > Source/WebCore/editing/VisibleSelection.cpp:544
> > -        m_extent = adjustPositionForStart(m_start, m_end.containerNode());
> > +        m_extent = adjustPositionForStart(m_start, endNode.ptr());
> 
> Ditto.

Fixed these in Fixedhttps://trac.webkit.org/changeset/236522.

[James Craig]

Comment on attachment 350882 [details]
Removed the dead code

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

> Source/WebKit/Shared/WebPreferences.yaml:1349
>  AriaReflectionEnabled:
>    type: bool
> -  defaultValue: true
> +  defaultValue: false

Why did this patch disable ARIA Reflection? Seems unrelated.

[Ryosuke Niwa]

Comment on attachment 350882 [details]
Removed the dead code

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

>> Source/WebKit/Shared/WebPreferences.yaml:1349
>> +  defaultValue: false
> 
> Why did this patch disable ARIA Reflection? Seems unrelated.

Indeed it doesn't seem related. I don't know how that happened.

[chris fleizach]

(In reply to Ryosuke Niwa from comment #19)
> Comment on attachment 350882 [details]
> Removed the dead code
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350882&action=review
> 
> >> Source/WebKit/Shared/WebPreferences.yaml:1349
> >> +  defaultValue: false
> > 
> > Why did this patch disable ARIA Reflection? Seems unrelated.
> 
> Indeed it doesn't seem related. I don't know how that happened.

Put up a patch

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