DevTools: text selection in console is broken when viewport is much smaller than content |
|||||||||||||||
Issue descriptionRepro steps: (1) https://jsfiddle.net/74afh7j1/ (2) Select the text and try to ctrl-C to trigger the copy handler Alternative repro in DevTools' console (1) Evaluate this in console for (var i=0; i < 200; i++) dir(i) (2) Resize the viewport height so that the scrollbar appears and there are a lot of messages outisde of the viewport (3) Make a selection by clicking and dragging that covers messages that were once outside of the viewport (4) Ctrl-C and paste somewhere else What is the expected result? We should be able to copy text that was not in the viewport when starting the selection What happens instead? We are not able to copy text that was moved out of the viewport. Narrow bisect to: https://chromium.googlesource.com/chromium/src/+/7afbab0f7619c89aa7a5896024d1628a59cc0ef1
,
Mar 9 2017
From what I can tell in crbug.com/690104 and crbug.com/695568 , the CL in the description was made to improve spec conformance. Copy event handlers registered on the document still work, but as the jsfiddle shows, handlers registered on normal elements no longer work. @mwrobel, @yosin, is this intended? It seems like a breaking change.
,
Mar 10 2017
I spoke about my patch with my team mate in opera, and we agreed that since my change breaks lot of current implementations (the example from this bug works against the spec. in firefox and in chrome before my change) we should suggest changing the spec. rather then changing the code to follow spec., which I've done today: https://github.com/w3c/clipboard-apis/issues/40 So at this point +2 (me and my team mate) for reverting my patch. p.s. If the change I suggested in spec. will be introduced, it will require a small change in chromium: rather then my latest path proposal: https://chromium.googlesource.com/chromium/src/+/7afbab0f7619c89aa7a5896024d1628a59cc0ef1%5E%21/#F3 it will be: Element* target = selection.isNone() ? frame().document()->activeElement() : associatedElementOf(selection.start());
,
Mar 10 2017
Okay, I've created a revert patchset here https://codereview.chromium.org/2745813002/ In the other thread about hterm, there was discussion about fixing hterm as opposed to reverting. Can we get confirmation to revert from yosin@ and vapier@? I'm not sure whether any newer dependent patches have landed in the last 2 weeks.
,
Mar 10 2017
i'd be happy to revert. i had not pushed my change to hterm as it caused other issues, and as i debugged the code, i wasn't confident that chrome was behaving correctly in the first place. namely, it seemed like i was getting an infinite loop where the copy event callback would trigger a copy event which would trigger the callback and it looped. i tried writing a simple test case, but chrome wouldn't infinite loop with that, and i haven't gotten back to it. when i looked at the original CL, it wasn't clear if we had good coverage. as part of the spec, can we make sure there's a compliance test suite that everyone agrees upon before we try making further changes in Chrome ?
,
Mar 11 2017
Gotcha. I've landed a revert here: https://codereview.chromium.org/2745813002/ To vapier@'s suggestion, maybe the predictability folks know if there's already an effort to do this better? It's a challenge to come up with compliance tests that cover the behavior used by websites out in the wild.
,
Mar 13 2017
,
Mar 13 2017
we going to cherry pick this back to M58 ?
,
Mar 13 2017
Given the blocked bug is marked as a release blocker for beta, landing the revert seems like the shortest path to moving forward on 58. If everyone is confident merging seems reasonable to me.
,
Mar 13 2017
I'm for merging this to 58 as well, but I'm not familiar with the editing system. I'd be more comfortable if someone with on the blink>editing team would also approve in case there are conflicts. There hasn't been a comment on this crbug or the hterm bug from an editing person since February, but since the revert has been on Canary. I'll request the merge. Editing folks, can you please comment?
,
Mar 13 2017
,
Mar 14 2017
Did you confirm the fix with shipped canary and its version?
,
Mar 14 2017
Verified the revert(https://codereview.chromium.org/2745813002/) on Windows-10, Mac OS 10.12.3 and Linux Ubuntu 14.04 on the latest M-59(59.0.3041.0). This is working as intended. Updating the OS and blocker label accordingly.
,
Mar 14 2017
yoichio@, yes, I confirmed the fix in the console yesterday on Canary 59.0.3040.0 before merging. Merge CL here: https://codereview.chromium.org/2745353002/
,
Mar 15 2017
,
Mar 15 2017
@ligimole, @abdulsyed, I don't believe the effect of this is enough to block Beta, removing the beta block label on this bug.
,
Mar 15 2017
Could someone on the editing team please take a look at adding a test for the core issue?
,
Mar 16 2017
Re-adding beta blocker label. After more consideration, I think the safest thing is to block beta until this fix is included, if possible.
,
Mar 21 2017
Just an update, the fix will be shipped with next Beta scheduled tomorrow(Wed 03/22). The patch is merged in - 58.0.3029.21 currently we have 58.0.3029.19 in production.
,
Apr 12 2017
Verified on build 9334.42.0 |
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by l...@chromium.org
, Mar 9 2017