Closed
Bug 1286882
Opened 8 years ago
Closed 6 years ago
Tapping SVG shape invokes text selection UI
Categories
(Core :: DOM: Selection, defect)
Tracking
()
RESOLVED
FIXED
mozilla63
People
(Reporter: glen, Assigned: TYLin)
References
(Blocks 3 open bugs, Regressed 2 open bugs)
Details
(Whiteboard: [webcompat:p1])
User Story
Business driver: Achieve tier-1 Google Search experience for Gecko on Android
Attachments
(3 files)
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160604131506
Firefox for Android
Steps to reproduce:
Using Firefox 48 beta on Android Lollipop, open the attached html file. Tap the circle or rectangle.
Actual results:
The UI associated with selecting text appears--"markers" at each end of the shape appear as if there's text in between, and there's a bar across the top with the check mark for copying text.
Expected results:
Nothing should have happened, as the user is not selecting text. Note: This does not happen in Android Firefox 47.0.
Reporter | ||
Updated•8 years ago
|
OS: Unspecified → Android
Hardware: Unspecified → ARM
Reporter | ||
Updated•8 years ago
|
Component: Untriaged → General
Product: Firefox → Firefox for Android
Comment 1•8 years ago
|
||
I'm able to reproduce this using:
Device:Galaxy Note 3(5.0), Galaxy Note 5(6.0),Xiaomi Pad 2(5.02)
Build:Beta - 48.0b6, Aurora - 49.0a2(2016-07-13), Nightly - 50.0a1(2016-07-14)
Status: UNCONFIRMED → NEW
status-firefox48:
--- → affected
status-firefox49:
--- → affected
status-firefox50:
--- → affected
Ever confirmed: true
Comment 2•8 years ago
|
||
Mark this looks to be fallout from the new text selection handles. 47 is unaffected.
tracking-fennec: --- → ?
status-firefox47:
--- → unaffected
tracking-firefox49:
--- → ?
tracking-firefox50:
--- → ?
Flags: needinfo?(markcapella)
Comment 3•8 years ago
|
||
My quick thought was dup of [0] ?
With the original UI scheme, it was our working assumption (at least as recently as [1]) that non-text bearing selections were ignoreable.
The rules did indeed change with AccessibileCarets, and we've let this bit of incompatiblility remain gray-ish.
Perhaps TYLin would care to comment further? I'm sure code can accommodate front-end one way or another.
[0] Bug 1221335 - AccessibleCarets seem to select some things (images, etc) that contain no text
[1] Bug 1119834 - Text selection action bar is shown on div element containing no text
Flags: needinfo?(markcapella)
Updated•8 years ago
|
Component: General → Text Selection
Flags: needinfo?(tlin)
Comment 4•8 years ago
|
||
Duping this makes sense.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Comment 5•8 years ago
|
||
Unsetting tracking nom for 49/50. We will track in the duplicate bug.
tracking-firefox49:
? → ---
tracking-firefox50:
? → ---
Assignee | ||
Updated•8 years ago
|
Blocks: AccessibleCaret
Component: Text Selection → Selection
Flags: needinfo?(tlin)
Product: Firefox for Android → Core
Assignee | ||
Comment 6•8 years ago
|
||
I think this is a separate issue from Bug 1221335. I can reproduce this issue on desktop browser.
When single-clicking on the SVG circle or rectangle, AccessibleCaretManager::OnSelectionChanged() is called with a non-collapsed selection range. That's why the carets are shown.
The question is: why is *single-clicking* on the SVG shape resulting a selection?
[1] http://searchfox.org/mozilla-central/rev/ff0e3782b1c44ffd1a68ef8e9e4f8be3866741e2/layout/base/AccessibleCaretManager.cpp#137
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
tracking-fennec: ? → +
Comment 8•8 years ago
|
||
Just to add, I am still seeing this issue on Firefox 49.0.2 on Android 6.0.1 (Samsung Galaxy S7).
Ting-Yu, do you have a chance to look at this again? I'm seeing it more frequently these days. Twitter Lite causes this, for instance.
Flags: needinfo?(tlin)
Assignee | ||
Comment 10•7 years ago
|
||
James, I won't be available in next three weeks, but I can take a look at this when I'm back. (Keep the needinfo as a reminder.)
Assignee | ||
Comment 11•7 years ago
|
||
The reason for this bug is that when tapping on an SVG shape, nsFrame::HandlePress() [1] will call nsFrameSelection::HandleClick() with offsets.content equals the shape's parent, and the offset to the shape.
For example, when tapping the circle in attachment 8770998 [details], the data is:
offsets.content = content svg@14259e4a0 xmlns="http://www.w3.org/2000/svg" id="svg-element" width="400" height="400"
offsets.StartOffset() = 3
offsets.EndOffset() = 4
Then, TakeFocus() in nsFrameSelection::HandleClick() will extend the range at [2] because the start offset and end offset is not equal. That's why AccessibleCaret shows.
[1] https://dxr.mozilla.org/mozilla-central/rev/fc194660762d1b92e1679d860a8bf41116d0f54f/layout/generic/nsFrame.cpp#4222
[2] https://dxr.mozilla.org/mozilla-central/rev/fc194660762d1b92e1679d860a8bf41116d0f54f/layout/generic/nsFrameSelection.cpp#1422-1424
Flags: needinfo?(tlin)
Comment 13•6 years ago
|
||
This causes interop issues as Chrome does not do this. That includes issues with GWS tier 1 interop, as tapping on SVG icons can bring up the selection interface (see bug 1473162 for example).
Flags: webcompat?
Whiteboard: [webcompat]
Comment 14•6 years ago
|
||
Ting-Yu, I just did some observations in Firefox and Chrome with the provided test-case and the following listener added in the web console:
>document.addEventListener("click", () => { setTimeout(() => { console.log(getSelection()) }, 1000) }, true);
Chrome seems to simply ignore the SVG shapes for the purposes of selection. I believe the attached patch will make Firefox do the same. Do you suspect that a patch like this seems reasonable? Or would a different approach be better?
With the patch, depending on where I click or tap in the SVG's content-area, I notice that Firefox makes a selection on either the SVG or text node, with range start==range end, but different values (1==1, 2==2, 5==5, etc).
On the other hand, Chrome always makes the selection on the text node, with range start==range end==14 if I don't click on the actual text itself. (And if the text node is removed entirely, it will select the SVG with start==end==1).
But I'm not sure if these minor-seeming differences are a big enough concern to worry about them.
Attachment #8990532 -
Flags: feedback?(aethanyc)
Updated•6 years ago
|
User Story: (updated)
Flags: webcompat? → webcompat+
Whiteboard: [webcompat] → [webcompat:p1]
Assignee | ||
Comment 15•6 years ago
|
||
Comment on attachment 8990532 [details] [diff] [review]
1286882.diff
Review of attachment 8990532 [details] [diff] [review]:
-----------------------------------------------------------------
In general, I think adjusting the offset representation to SVGGeometry frame is the correct approach to avoid selecting it. However, if we're still using SVGGeometry's parent with some offset (x, x+1), rather than SVGGeometry itself, to represent the range, we'll inevitably make a new non-collapse range over the SVGGeometry when clicking on it.
I've tested your patch on desktop, it'll still select the SVG element when there's an active selection exists. For example, in the test case in comment 0:
1. Double click on the rectangle to make a selection.
2. Single click on the circle.
I'm seeing the selection handle show on the circle.
I'm testing another approach, and I'll post it shortly.
Attachment #8990532 -
Flags: feedback?(aethanyc)
Comment hidden (obsolete) |
Updated•6 years ago
|
Attachment #8990847 -
Attachment description: Bug 1286882 - Change the range representation of SVGGeometryFrame. → Bug 1286882 - Make SVG basic shape elements unselectable.
Comment 17•6 years ago
|
||
Comment on attachment 8990847 [details]
Bug 1286882 - Make SVG basic shape elements unselectable.
Daniel Holbert [:dholbert] has approved the revision.
https://phabricator.services.mozilla.com/D2038
Attachment #8990847 -
Flags: review+
Comment 18•6 years ago
|
||
Pushed by aethanyc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2de43bdd92a8
Make SVG basic shape elements unselectable. r=dholbert
Comment 19•6 years ago
|
||
I like this solution, but it does mean that users can over-ride the selection rule while Chrome does not seem to allow that.
That is, if I make a <circle style="user-select:all">, Firefox will select it on a long tap whereas Chrome will not (taking CSS prefixes into account, of course).
This doesn't strike me as a big deal, but to be fair neither did the bug this patch fixes.
Thoughts?
Flags: needinfo?(aethanyc)
Comment 20•6 years ago
|
||
Backed out changeset 2de43bdd92a8 (bug 1286882) for failures on test_text_selection.html
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2de43bdd92a8a37aa98341e3f576b7951513bf1b
Backout link: https://hg.mozilla.org/integration/autoland/rev/46563760cbb9214f18fcb177d12d09ed94e43f4d
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=187721617&repo=autoland
[task 2018-07-12T01:56:15.804Z] 01:56:15 INFO - TEST-START | dom/svg/test/test_text_selection.html
[task 2018-07-12T01:56:15.941Z] 01:56:15 INFO - TEST-INFO | started process screentopng
[task 2018-07-12T01:56:16.500Z] 01:56:16 INFO - TEST-INFO | screentopng: exit 0
[task 2018-07-12T01:56:16.501Z] 01:56:16 INFO - Buffered messages logged at 01:56:15
[task 2018-07-12T01:56:16.501Z] 01:56:16 INFO - TEST-PASS | dom/svg/test/test_text_selection.html | selecting entire simple text
[task 2018-07-12T01:56:16.501Z] 01:56:16 INFO - TEST-PASS | dom/svg/test/test_text_selection.html | deselecting by clicking on text
[task 2018-07-12T01:56:16.506Z] 01:56:16 INFO - TEST-PASS | dom/svg/test/test_text_selection.html | selecting part of simple text
[task 2018-07-12T01:56:16.506Z] 01:56:16 INFO - Buffered messages finished
[task 2018-07-12T01:56:16.506Z] 01:56:16 INFO - TEST-UNEXPECTED-FAIL | dom/svg/test/test_text_selection.html | deselecting by clicking outside text - got "hello", expected ""
[task 2018-07-12T01:56:16.512Z] 01:56:16 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:312:5
[task 2018-07-12T01:56:16.517Z] 01:56:16 INFO - selection_is@dom/svg/test/test_text_selection.html:52:3
[task 2018-07-12T01:56:16.518Z] 01:56:16 INFO - deselect@dom/svg/test/test_text_selection.html:59:3
[task 2018-07-12T01:56:16.519Z] 01:56:16 INFO - testSelection@dom/svg/test/test_text_selection.html:83:3
[task 2018-07-12T01:56:16.522Z] 01:56:16 INFO - TEST-PASS | dom/svg/test/test_text_selection.html | selecting entire simple text by dragging around it
[task 2018-07-12T01:56:16.523Z] 01:56:16 INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-07-12T01:56:16.524Z] 01:56:16 INFO - TEST-UNEXPECTED-FAIL | dom/svg/test/test_text_selection.html | deselecting by clicking outside text - got "hello there", expected ""
[task 2018-07-12T01:56:16.528Z] 01:56:16 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:312:5
[task 2018-07-12T01:56:16.529Z] 01:56:16 INFO - selection_is@dom/svg/test/test_text_selection.html:52:3
[task 2018-07-12T01:56:16.534Z] 01:56:16 INFO - deselect@dom/svg/test/test_text_selection.html:59:3
[task 2018-07-12T01:56:16.539Z] 01:56:16 INFO - testSelection@dom/svg/test/test_text_selection.html:88:3
[task 2018-07-12T01:56:16.540Z] 01:56:16 INFO - TEST-PASS | dom/svg/test/test_text_selection.html | selecting part of simple text by dragging above it
[task 2018-07-12T01:56:16.548Z] 01:56:16 INFO - Not taking screenshot here: see the one that was previously logged
Updated•6 years ago
|
Assignee: nobody → aethanyc
Assignee | ||
Comment 21•6 years ago
|
||
(In reply to twisniewski from comment #19)
> I like this solution, but it does mean that users can over-ride the
> selection rule while Chrome does not seem to allow that.
>
> That is, if I make a <circle style="user-select:all">, Firefox will select
> it on a long tap whereas Chrome will not (taking CSS prefixes into account,
> of course).
Yes, you are right.
> This doesn't strike me as a big deal, but to be fair neither did the bug
> this patch fixes.
>
> Thoughts?
We use the same technique to make <canvas> [1] and <video> [2] unselectable. I think it's actually good to give the user the freedom to override that in order to do whatever they want. Making them "-moz-user-select: none ! important" seems too strong to me. We'd better keep CSS rules in our UA as flexible as they could unless they're critical to the correctness of layout frame tree structure or rendering.
[1] https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/layout/style/res/html.css#705-707
[2] https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/layout/style/TopLevelVideoDocument.css#27
Flags: needinfo?(aethanyc)
Comment 23•6 years ago
|
||
Pushed by aethanyc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/90453be111d2
Make SVG basic shape elements unselectable. r=dholbert
Comment 24•6 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 8 years ago → 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment 25•6 years ago
|
||
Thanks Stefan for linking my bug report to this one.
And thanks to everyone else involved in fixing the bug.
I just tested it in the Nightly to confirm, whether it fixes everything.
And it kinda does, but not all cases, that I've found.
YouTube, the Whatsapp blog, festify.us and github were fixed by it.
Amazon and Common Voice are still affected though.
And at least Common Voice doesn't involve an svg as far as I can see.
Just "yes" and "no" on buttons
I would be glad, if you could take a look https://bugzilla.mozilla.org/show_bug.cgi?id=1464792
and reopen it again :)
Comment 26•6 years ago
|
||
Is this something we should consider uplifting to Beta or can it ride the trains?
status-firefox61:
--- → wontfix
status-firefox62:
--- → affected
Flags: needinfo?(aethanyc)
Flags: in-testsuite+
Assignee | ||
Comment 27•6 years ago
|
||
I'd want it to ride the trains because patches related to selection are easy to have regression.
Flags: needinfo?(aethanyc)
Updated•6 years ago
|
Updated•4 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•