Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2856933006:
DevTools: fix text offset when selection ends on expand triangle (Closed)
|
Created:
3 years, 7 months ago by luoe Modified:
3 years, 7 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, pfeldman Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: fix text offset when selection ends on expand triangle
A user's text selection may start/end on a non-TextNode with no text content,
for example the expand triangle icon. In these cases, getSelection()'s
anchor/focus node may be located before the first TextNode in a console message.
These cases break ConsoleViewport._textOffsetInNode(), which traverses starting
from the first TextNode. This CL addresses it by moving the target node to the
next TextNode.
BUG=717286
Review-Url: https://codereview.chromium.org/2856933006
Cr-Commit-Position: refs/heads/master@{#470442}
Committed: https://chromium.googlesource.com/chromium/src/+/56d8d770469ff02ac1521f49addaae48863a05f8
Patch Set 1 #
Total comments: 10
Patch Set 2 : ac #
Total comments: 2
Patch Set 3 : ac #
Total comments: 2
Patch Set 4 : ac #
Messages
Total messages: 22 (9 generated)
Description was changed from ========== DevTools: fix text offset when selection ends on expand triangle BUG=717286 ========== to ========== DevTools: fix text offset when selection ends on expand triangle A user's text selection may start/end on a non-TextNode with no text content, for example the expand triangle icon. In these cases, getSelection()'s anchor/focus node may be located before the first TextNode in a console message. These cases break ConsoleViewport._textOffsetInNode(), which traverses starting from the first TextNode. This CL addresses it by moving the target node to the next TextNode. BUG=717286 ==========
luoe@chromium.org changed reviewers: + einbinder@chromium.org, lushnikov@chromium.org
Please take a look. The repro case is a little tricky because it requires letting go of the mouse exactly on the triangle.
https://codereview.chromium.org/2856933006/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/inspector/console/console-copy-truncated-text-expected.txt (right): https://codereview.chromium.org/2856933006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector/console/console-copy-truncated-text-expected.txt:163: Highlighted 20 matches Why did this go up by 2? Isn't there just one new message with www? https://codereview.chromium.org/2856933006/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2856933006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:450: _textOffsetInNode(itemElement, container, offset) { These names are confusing because the itemElement contains the container. Maybe call it selection node? https://codereview.chromium.org/2856933006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:463: if (container.textContent.length === 0 && container.nodeType !== Node.TEXT_NODE) { !container.textContent.length? I don't like the idea of selection being in an empty text node https://codereview.chromium.org/2856933006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:466: container = nextTextNode; This works, but it feels uncomfortable to not have an else here
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2856933006/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/inspector/console/console-copy-truncated-text-expected.txt (right): https://codereview.chromium.org/2856933006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector/console/console-copy-truncated-text-expected.txt:163: Highlighted 20 matches On 2017/05/03 01:07:48, einbinder wrote: > Why did this go up by 2? Isn't there just one new message with www? The 'command' message that has all the console.log's also gets a search match. It should be clearer if we evaluateInPage instead of evaluateInConsole. https://codereview.chromium.org/2856933006/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2856933006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:450: _textOffsetInNode(itemElement, container, offset) { On 2017/05/03 01:07:48, einbinder wrote: > These names are confusing because the itemElement contains the container. Maybe > call it selection node? Done. https://codereview.chromium.org/2856933006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:463: if (container.textContent.length === 0 && container.nodeType !== Node.TEXT_NODE) { On 2017/05/03 01:07:48, einbinder wrote: > !container.textContent.length? I don't like the idea of selection being in an > empty text node Done. https://codereview.chromium.org/2856933006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:466: container = nextTextNode; On 2017/05/03 01:07:48, einbinder wrote: > This works, but it feels uncomfortable to not have an else here I can see that it seems strange at first on first glance, "If we're supposed to move the container, wouldn't something break if that fails?" But we don't actually need to change the container if there is no next text node. Perhaps it reads better as selectionNode = selectionNode.traverseNextTextNode(itemElement) || selectionNode; ?
lgtm https://codereview.chromium.org/2856933006/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/inspector/console/console-copy-truncated-text-expected.txt (right): https://codereview.chromium.org/2856933006/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector/console/console-copy-truncated-text-expected.txt:163: Highlighted 20 matches On 2017/05/03 at 02:58:39, luoe wrote: > On 2017/05/03 01:07:48, einbinder wrote: > > Why did this go up by 2? Isn't there just one new message with www? > > The 'command' message that has all the console.log's also gets a search match. > > It should be clearer if we evaluateInPage instead of evaluateInConsole. Ah that makes sense. https://codereview.chromium.org/2856933006/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2856933006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:466: container = nextTextNode; On 2017/05/03 at 02:58:39, luoe wrote: > On 2017/05/03 01:07:48, einbinder wrote: > > This works, but it feels uncomfortable to not have an else here > > I can see that it seems strange at first on first glance, "If we're supposed to move the container, wouldn't something break if that fails?" But we don't actually need to change the container if there is no next text node. > > Perhaps it reads better as > selectionNode = selectionNode.traverseNextTextNode(itemElement) || selectionNode; > ? I guess its fine the way it was.
https://codereview.chromium.org/2856933006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2856933006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:468: while ((node = node.traverseNextTextNode(itemElement)) && !node.isSelfOrDescendant(selectionNode)) let's use node.traverseNextNode(itemElement) to fix the stop condition. You won't need the cryptic if-condition in this case.
ptal https://codereview.chromium.org/2856933006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2856933006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:468: while ((node = node.traverseNextTextNode(itemElement)) && !node.isSelfOrDescendant(selectionNode)) On 2017/05/05 23:31:57, lushnikov wrote: > let's use node.traverseNextNode(itemElement) to fix the stop condition. > You won't need the cryptic if-condition in this case. Done.
lgtm https://codereview.chromium.org/2856933006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2856933006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:465: var isValidTextNode = node.nodeType === Node.TEXT_NODE && !nonTextTags[node.parentElement.nodeName]; nit: you can write this without having nonTextTags and isValidTextNode variables. I think that the following is cleaner: if (node.nodeType !== Node.TEXT_NODE || node.parentElement.nodeName === 'SCRIPT' || node.parentElement.nodeName === 'STYLE') continue; chars += Components.Linkifier.untruncatedNodeText(node).length;
Thanks! https://codereview.chromium.org/2856933006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2856933006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:465: var isValidTextNode = node.nodeType === Node.TEXT_NODE && !nonTextTags[node.parentElement.nodeName]; On 2017/05/09 00:39:40, lushnikov wrote: > nit: you can write this without having nonTextTags and isValidTextNode > variables. > I think that the following is cleaner: > > if (node.nodeType !== Node.TEXT_NODE || node.parentElement.nodeName === 'SCRIPT' > || node.parentElement.nodeName === 'STYLE') > continue; > chars += Components.Linkifier.untruncatedNodeText(node).length; Done.
The CQ bit was checked by luoe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from einbinder@chromium.org, lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/2856933006/#ps80001 (title: "ac")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by luoe@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494361373709250,
"parent_rev": "2e0ee81a6380db6c714980a30a53a8c002ff46e7", "commit_rev":
"56d8d770469ff02ac1521f49addaae48863a05f8"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: fix text offset when selection ends on expand triangle A user's text selection may start/end on a non-TextNode with no text content, for example the expand triangle icon. In these cases, getSelection()'s anchor/focus node may be located before the first TextNode in a console message. These cases break ConsoleViewport._textOffsetInNode(), which traverses starting from the first TextNode. This CL addresses it by moving the target node to the next TextNode. BUG=717286 ========== to ========== DevTools: fix text offset when selection ends on expand triangle A user's text selection may start/end on a non-TextNode with no text content, for example the expand triangle icon. In these cases, getSelection()'s anchor/focus node may be located before the first TextNode in a console message. These cases break ConsoleViewport._textOffsetInNode(), which traverses starting from the first TextNode. This CL addresses it by moving the target node to the next TextNode. BUG=717286 Review-Url: https://codereview.chromium.org/2856933006 Cr-Commit-Position: refs/heads/master@{#470442} Committed: https://chromium.googlesource.com/chromium/src/+/56d8d770469ff02ac1521f49adda... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/56d8d770469ff02ac1521f49adda... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
