Bug 151380 - Selection should work across shadow boundary when initiated by a mouse drag
Summary: Selection should work across shadow boundary when initiated by a mouse drag
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 148695
  Show dependency treegraph
 
Reported: 2015-11-17 21:52 PST by Ryosuke Niwa
Modified: 2019-02-14 01:40 PST (History)
10 users (show)

See Also:


Attachments
WIP (2.29 KB, patch)
2018-09-25 02:09 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Adds new behavior (35.56 KB, patch)
2018-09-26 02:16 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (45.76 KB, patch)
2018-09-26 12:27 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Removed the dead code (45.42 KB, patch)
2018-09-26 12:30 PDT, Ryosuke Niwa
wenson_hsieh: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2015-11-17 21:52:21 PST
User should be able to select contents across a shadow boundary.
Comment 1 Radar WebKit Bug Importer 2016-01-26 23:51:16 PST
<rdar://problem/24363872>
Comment 2 Ryosuke Niwa 2018-09-25 02:09:27 PDT
Created attachment 350744 [details]
WIP
Comment 3 EWS Watchlist 2018-09-25 02:34:45 PDT
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.
Comment 4 Ryosuke Niwa 2018-09-26 02:16:10 PDT
Created attachment 350851 [details]
Adds new behavior
Comment 5 Antti Koivisto 2018-09-26 02:46:56 PDT
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?
Comment 6 Ryosuke Niwa 2018-09-26 02:53:19 PDT
(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 :(
Comment 7 Antti Koivisto 2018-09-26 02:54:13 PDT
Comment on attachment 350851 [details]
Adds new behavior

Ok, r=me without the commented out part.
Comment 8 Ryosuke Niwa 2018-09-26 02:54:30 PDT
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.
Comment 9 Ryosuke Niwa 2018-09-26 02:54:56 PDT
Thanks for the review! Would you still stand by your r=me if I disabled the feature by default??
Comment 10 Antti Koivisto 2018-09-26 02:58:46 PDT
Sure!
Comment 11 Ryosuke Niwa 2018-09-26 12:27:11 PDT
Created attachment 350881 [details]
Patch for landing
Comment 12 Ryosuke Niwa 2018-09-26 12:27:50 PDT
Comment on attachment 350881 [details]
Patch for landing

Wait for EWS.
Comment 13 Ryosuke Niwa 2018-09-26 12:30:25 PDT
Created attachment 350882 [details]
Removed the dead code
Comment 14 Ryosuke Niwa 2018-09-26 12:32:14 PDT
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.
Comment 15 Wenson Hsieh 2018-09-26 12:43:56 PDT
Comment on attachment 350882 [details]
Removed the dead code

r=me (and antti)
Comment 16 Ryosuke Niwa 2018-09-26 13:02:59 PDT
Committed r236519: <https://trac.webkit.org/changeset/236519>
Comment 17 Ryosuke Niwa 2018-09-26 13:33:37 PDT
(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.
Comment 18 James Craig 2019-02-14 01:19:28 PST
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.
Comment 19 Ryosuke Niwa 2019-02-14 01:22:42 PST
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.
Comment 20 chris fleizach 2019-02-14 01:40:48 PST
(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