Bug 151380

Summary: Selection should work across shadow boundary when initiated by a mouse drag
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: cfleizach, commit-queue, darin, enrica, ews-watchlist, jcraig, koivisto, megan_gardner, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 148695    
Attachments:
Description Flags
WIP
none
Adds new behavior
none
Patch for landing
none
Removed the dead code wenson_hsieh: review+

Ryosuke Niwa
Reported 2015-11-17 21:52:21 PST
User should be able to select contents across a shadow boundary.
Attachments
WIP (2.29 KB, patch)
2018-09-25 02:09 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Adds new behavior (35.56 KB, patch)
2018-09-26 02:16 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Patch for landing (45.76 KB, patch)
2018-09-26 12:27 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Removed the dead code (45.42 KB, patch)
2018-09-26 12:30 PDT, Ryosuke Niwa 		wenson_hsieh: review+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2016-01-26 23:51:16 PST
<rdar://problem/24363872>
Ryosuke Niwa
Comment 2 2018-09-25 02:09:27 PDT
Created attachment 350744 [details] WIP
EWS Watchlist
Comment 3 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.
Ryosuke Niwa
Comment 4 2018-09-26 02:16:10 PDT
Created attachment 350851 [details] Adds new behavior
Antti Koivisto
Comment 5 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?
Ryosuke Niwa
Comment 6 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 :(
Antti Koivisto
Comment 7 2018-09-26 02:54:13 PDT
Comment on attachment 350851 [details] Adds new behavior Ok, r=me without the commented out part.
Ryosuke Niwa
Comment 8 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.
Ryosuke Niwa
Comment 9 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??
Antti Koivisto
Comment 10 2018-09-26 02:58:46 PDT
Sure!
Ryosuke Niwa
Comment 11 2018-09-26 12:27:11 PDT
Created attachment 350881 [details] Patch for landing
Ryosuke Niwa
Comment 12 2018-09-26 12:27:50 PDT
Comment on attachment 350881 [details] Patch for landing Wait for EWS.
Ryosuke Niwa
Comment 13 2018-09-26 12:30:25 PDT
Created attachment 350882 [details] Removed the dead code
Ryosuke Niwa
Comment 14 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.
Wenson Hsieh
Comment 15 2018-09-26 12:43:56 PDT
Comment on attachment 350882 [details] Removed the dead code r=me (and antti)
Ryosuke Niwa
Comment 16 2018-09-26 13:02:59 PDT
Committed r236519: <https://trac.webkit.org/changeset/236519>
Ryosuke Niwa
Comment 17 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.
James Craig
Comment 18 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.
Ryosuke Niwa
Comment 19 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.
chris fleizach
Comment 20 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
Note You need to log in before you can comment on or make changes to this bug.