Summary: | REGRESSION(r238599): Web Inspector: Clicking on text doesn't move text caret when editing innerHTML/tagName/attribute | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikita Vasilyev <nvasilyev> | ||||||||||||
Component: | Web Inspector | Assignee: | Matt Baker <mattbaker> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | chris, commit-queue, hi, inspector-bugzilla-changes, mattbaker, webkit-bug-importer | ||||||||||||
Priority: | P1 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=208310 | ||||||||||||||
Attachments: |
|
Description
Nikita Vasilyev
2018-12-12 19:09:50 PST
Created attachment 359294 [details]
Patch
Regressed in https://trac.webkit.org/changeset/238599. Looks good, I'd r+. In r238599, event.preventDefault() was called on mousedown even when DOMTreeElement was being edited. This patch fixes it. Created attachment 359298 [details]
[Video] Multiple elements selected
This patch caused multiple elements selected by mistake to come back.
(In reply to Nikita Vasilyev from comment #5) > Created attachment 359298 [details] > [Video] Multiple elements selected > > This patch caused multiple elements selected by mistake to come back. I don't have reliable steps to reproduce it. I got into this state on the video after reloading https://webkit.org. I haven't seen this bug for a few weeks so I assumed it's a regression in your patch. (In reply to Nikita Vasilyev from comment #6) > (In reply to Nikita Vasilyev from comment #5) > > Created attachment 359298 [details] > > [Video] Multiple elements selected > > > > This patch caused multiple elements selected by mistake to come back. > > I don't have reliable steps to reproduce it. I got into this state on the > video after reloading https://webkit.org. I haven't seen this bug for a few > weeks so I assumed it's a regression in your patch. I'm not able to reproduce this. I tried clicking around various DOM nodes, using up/down arrows, expanding, etc. Could you adds steps? Comment on attachment 359294 [details]
Patch
I don't have the exact steps. I'll file a bug if I come up with a reliable way to reproduce it.
I'm not sure it was related to the patch, so I'm reseting it back to "r?".
Created attachment 359804 [details]
[Video] Unable to change selection with mouse
I'm trying to click/drag in the video, and the selection doesn't respond. Using the keyboard works as expected.
Comment on attachment 359294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359294&action=review r-, see attachment 359804 [details] (comment #9) > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:649 > // Prevent selecting the nearest word on double click. Should we only be doing this when we aren't editing? I can imagine that it would be very useful to double-click to select a specific class name (in the space-separated list) when editing. Comment on attachment 359294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359294&action=review >> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:649 >> // Prevent selecting the nearest word on double click. > > Should we only be doing this when we aren't editing? I can imagine that it would be very useful to double-click to select a specific class name (in the space-separated list) when editing. Originally (before multi-selection was introduced in TreeOutline) this was _only_ done when not editing: selectOnMouseDown(event) { super.selectOnMouseDown(event); if (this._editing) return; // Prevent selecting the nearest word on double click. if (event.detail >= 2) event.preventDefault(); } Created attachment 360165 [details]
[Video] Change selection with mouse
I can't reproduce the selection bug you demonstrated. Tried with the patch applied on top of 86ca8ae2d1c.
(In reply to Matt Baker from comment #12) > Created attachment 360165 [details] > [Video] Change selection with mouse > > I can't reproduce the selection bug you demonstrated. Tried with the patch > applied on top of 86ca8ae2d1c. Do you have any settings (e.g. "Press Tab to highlight each item on a webpage", or some accessibility setting in System Preferences)? I remember we had a issue with that in the past (tabbing through items in the Styles sidebar), so maybe this is also related to that. (In reply to Devin Rousso from comment #13) > (In reply to Matt Baker from comment #12) > > Created attachment 360165 [details] > > [Video] Change selection with mouse > > > > I can't reproduce the selection bug you demonstrated. Tried with the patch > > applied on top of 86ca8ae2d1c. > Do you have any settings (e.g. "Press Tab to highlight each item on a > webpage", or some accessibility setting in System Preferences)? I remember > we had a issue with that in the past (tabbing through items in the Styles > sidebar), so maybe this is also related to that. I have "Tap to click" enabled in my trackpad settings but that doesn't matter. (In reply to Matt Baker from comment #12) > Created attachment 360165 [details] > [Video] Change selection with mouse > > I can't reproduce the selection bug you demonstrated. Tried with the patch > applied on top of 86ca8ae2d1c. I can't reproduce this either. Devin, did you successfully apply the patch? Comment on attachment 359294 [details]
Patch
r=me
Interesting. I reapplied this, and it seems to work fine now 🤔
Comment on attachment 359294 [details] Patch Clearing flags on attachment: 359294 Committed r240819: <https://trac.webkit.org/changeset/240819> All reviewed patches have been landed. Closing bug. *** Bug 194195 has been marked as a duplicate of this bug. *** |