Closed Bug 1286882 Opened 3 years ago Closed Last year

Tapping SVG shape invokes text selection UI

Categories

(Core :: DOM: Selection, defect)

48 Branch
ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox47 --- unaffected
firefox48 --- wontfix
firefox49 --- wontfix
fennec + ---
firefox50 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: glen, Assigned: TYLin)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [webcompat:p1])

User Story

Business driver: Achieve tier-1 Google Search experience for Gecko on Android

Attachments

(3 files)

Attached file svgIssue.html
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.
OS: Unspecified → Android
Hardware: Unspecified → ARM
Component: Untriaged → General
Product: Firefox → Firefox for Android
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
Ever confirmed: true
Mark this looks to be fallout from the new text selection handles. 47 is unaffected.
tracking-fennec: --- → ?
Flags: needinfo?(markcapella)
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)
Component: General → Text Selection
Flags: needinfo?(tlin)
Duping this makes sense.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1221335
Unsetting tracking nom for 49/50. We will track in the duplicate bug.
Component: Text Selection → Selection
Flags: needinfo?(tlin)
Product: Firefox for Android → Core
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: ? → +
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)
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.)
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)
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]
Attached patch 1286882.diffSplinter Review
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)
User Story: (updated)
Flags: webcompat? → webcompat+
Whiteboard: [webcompat] → [webcompat:p1]
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)
Attachment #8990847 - Attachment description: Bug 1286882 - Change the range representation of SVGGeometryFrame. → Bug 1286882 - Make SVG basic shape elements unselectable.
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+
Pushed by aethanyc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2de43bdd92a8
Make SVG basic shape elements unselectable. r=dholbert
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)
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
Assignee: nobody → aethanyc
(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)
Duplicate of this bug: 1464792
Pushed by aethanyc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/90453be111d2
Make SVG basic shape elements unselectable. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/90453be111d2
Status: REOPENED → RESOLVED
Closed: 3 years agoLast year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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 :)
Is this something we should consider uplifting to Beta or can it ride the trains?
Flags: needinfo?(aethanyc)
Flags: in-testsuite+
I'd want it to ride the trains because patches related to selection are easy to have regression.
Flags: needinfo?(aethanyc)
You need to log in before you can comment on or make changes to this bug.