1286882 - Tapping SVG shape invokes text selection UI

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Glen Reesor]

Attachment: 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.

Comment 1

[Alex Caziuc]

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)

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

Mark this looks to be fallout from the new text selection handles. 47 is unaffected.

Comment 3

[Mark Capella [:capella]]

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

Comment 4

[Kevin Brosnan [Ex-Mozilla]]

Duping this makes sense.

Comment 5

[Marcia Knous [:marcia]]

Unsetting tracking nom for 49/50. We will track in the duplicate bug.

Comment 6

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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

Comment 8

[Ian Devlin]

Just to add, I am still seeing this issue on Firefox 49.0.2 on Android 6.0.1 (Samsung Galaxy S7).

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 10

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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.)

Comment 11

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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

Comment 13

[Thomas Wisniewski [:twisniewski]]

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).

Comment 14

[Thomas Wisniewski [:twisniewski]]

Attachment: 1286882.diff

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.

Comment 15

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

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.

Comment 16

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1286882 - Make SVG basic shape elements unselectable.

We are using (parent, index, index +1) form to represent the range of an
SVGGeometryFrame. This will create a non-collapse range when we click on it.

By changing the range representation of SVGGeometryFrame to be like a block
frame, it becomes a collapse range when we click on it.

Comment 17

[Phabricator Automation]

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2de43bdd92a8
Make SVG basic shape elements unselectable. r=dholbert

Comment 19

[Thomas Wisniewski [:twisniewski]]

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?

Comment 20

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 21

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

(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

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90453be111d2
Make SVG basic shape elements unselectable. r=dholbert

Comment 24

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/90453be111d2

Comment 25

[Djfe]

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

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider uplifting to Beta or can it ride the trains?

Comment 27

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

I'd want it to ride the trains because patches related to selection are easy to have regression.