1217586 - [e10s] Right-clicking, Cmd+Clicking doesn't work on links in inline SVG

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Taylor Hunt]

Attachment: inline-svg-links.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20151022004116

Steps to reproduce:

Attempt to open the link inside the inline SVG in a new tab. Right click, Cmd+click, keyboard focus + Cmd + Enter, nothing.


Actual results:

When right-clicking, the generic page context menu appears.

When Cmd+clicking or focus + Cmd + Enter, the link doesn't open at all.


Expected results:

SVG links should probably behave like normal HTML links, as far as user interface goes.

Comment 1

[Taylor Hunt]

Might this have something to do with the recent change to classify <a>'s without href as non-interactive content?

https://bugzilla.mozilla.org/show_bug.cgi?id=1167816
https://developer.mozilla.org/en-US/Firefox/Releases/41#HTML

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

Regression window:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=90ea370e792f&tochange=b317389acc04

Regressed by: 4e1245f345fa	Felipe Gomes — Bug 1163422. r=Gijs

Comment 3

[:Gijs (he/him)]

(In reply to Alice0775 White from comment #2)
> [Tracking Requested - why for this release]:
> 
> [Tracking Requested - why for this release]:
> 
> Regression window:
> https://hg.mozilla.org/integration/fx-team/
> pushloghtml?fromchange=90ea370e792f&tochange=b317389acc04
> 
> Regressed by: 4e1245f345fa	Felipe Gomes — Bug 1163422. r=Gijs

This doesn't make sense. That bug was first fixed for 39, as well as ESR. Why does this not affect 42 and below? Is this e10s-specific?

Comment 4

[Alice0775 White]

(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to Alice0775 White from comment #2)
> > [Tracking Requested - why for this release]:
> > 
> > [Tracking Requested - why for this release]:
> > 
> > Regression window:
> > https://hg.mozilla.org/integration/fx-team/
> > pushloghtml?fromchange=90ea370e792f&tochange=b317389acc04
> > 
> > Regressed by: 4e1245f345fa	Felipe Gomes — Bug 1163422. r=Gijs
> 
> This doesn't make sense. That bug was first fixed for 39, as well as ESR.
> Why does this not affect 42 and below? Is this e10s-specific?

Yes this is e10s specific bug.
I can confirm it works as expected with disabled e10s.

Comment 5

[Liz Henry (:lizzard)]

Tracking for 43+ since this is a regression and 43 is currently affected. We could still take a patch for this in 43. e10s will not be enabled for 43 except in some a/b testing.

Comment 6

[Liz Henry (:lizzard)]

Wontfixing for 43. I'm not sure if this should be a menus issue or SVG or something else. 
We could still take a patch but since no one is working on this and e10s is off by default on beta 43, it seems more realistic to try for a fix aimed at 44.

Comment 7

[Daniel Holbert [:dholbert]]

This bug is caused by the code added in bug https://hg.mozilla.org/integration/fx-team/rev/4e1245f345fa failing. That code is:
>       try {
>         BrowserUtils.urlSecurityCheck(href, node.ownerDocument.nodePrincipal);
>       } catch (e) {
>         return;
>       }

and the problem is, we're throwing because 'node' is null. If I add "dump(e)" to the catch clause, I see:
> TypeError: node is null

The null |node| comes from a call to "_hrefAndLinkNodeForClickEvent", higher up in this file.  That function relies on a helper called "isHTMLLink", which as you might expect from its name, does not expect SVG links.

So, I think the bug here lies in _hrefAndLinkNodeForClickEvent -- it needs to be broadened to not be HTML-specific.  Or alternately, we need to be passing a different variable (maybe event.target?) for this security check -- not sure.

Felipe, any chance you can take a look?

Comment 8

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1217586 - fix modifier click / enter on SVG links by passing node principal, r?felipe

Bug 1217586 - fix modifier click / enter on SVG links by passing node principal, r?felipe

Comment 9

[:Gijs (he/him)]

Note that this doesn't fix the context menu case. That's also broken outside of e10s, so I think it's broken for different reasons. I'll file a followup bug assuming this bug's fix is deemed OK. It's likely we just need similar code in the context menu code.

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8686684 [details]
MozReview Request: Bug 1217586 - fix modifier click / enter on SVG links by passing node principal, r?felipe

https://reviewboard.mozilla.org/r/25021/#review22595

Comment 11

[:Felipe Gomes (needinfo for replies!)]

(In reply to :Gijs Kruitbosch from comment #9)
> Note that this doesn't fix the context menu case. That's also broken outside
> of e10s, so I think it's broken for different reasons. 

Do you know if this was broken outside of e10s, even before bug 1163422?

Comment 12

[:Gijs (he/him)]

(In reply to :Felipe Gomes from comment #11)
> (In reply to :Gijs Kruitbosch from comment #9)
> > Note that this doesn't fix the context menu case. That's also broken outside
> > of e10s, so I think it's broken for different reasons. 
> 
> Do you know if this was broken outside of e10s, even before bug 1163422?

I checked 38.0a1 nightly from January 13, and that was broken in non-e10s, so yes.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/bece86c18efc

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bece86c18efc

Comment 15

[Cornel Ionce [:noni]]

This was only fixed for the "keyboard focus + Cmd + Enter" case on Mac OS X 10.11 using latest Nightly.

Marking issue verified, since I've filled bug 1227116 for the "Cmd+click" case.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Gijs, should we uplift this to Aurora44?

Comment 17

[:Gijs (he/him)]

(In reply to Ritu Kothari (:ritu) from comment #16)
> Gijs, should we uplift this to Aurora44?

Yes, but we should include the fix in bug 1227116. I'll combine the patches and request uplift on those tomorrow. Leaving needinfo to remind myself.

Comment 18

[:Gijs (he/him)]

Attachment: Patch for aurora (combined with bug 1227116

Approval Request Comment
[Feature/regressing bug #]: bug 1163422
[User impact if declined]: link clicks with modifiers on SVG links don't work
[Describe test coverage new/current, TreeHerder]: nope :-(
[Risks and why]: pretty low, small localized patch, e10s-only
[String/UUID change made/needed]: no

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8694715 [details]
Patch for aurora (combined with bug 1227116

Both the fixes have been on Nightly for more than a week. Fix for bug 1227116 has been verified, waiting for verification on bug 1227116. Let's uplift to Aurora44.

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b7632a320e0b

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/b7632a320e0b

Comment 22

[Forhad Hossain]

I have reproduced the bug in Nightly 44.0a1 (2015-10-22)(Build ID:20151022030546) with the instruction from comment 0 and linux lite 32 bit

Bug is fixed now on Latest Developer Edition 44.0a2 (2015-12-13)(Build ID:20151213004008)
User Agent : Mozilla/5.0 (X11; Linux i686; rv:44.0) Gecko/20100101 Firefox/44.0

& Latest Nightly 45.0a1 (2015-12-12)(Build ID:20151212030222)
User Agent : Mozilla/5.0 (X11; Linux i686; rv:45.0) Gecko/20100101 Firefox/45.0

[testday-20151211]

Comment 23

[Taylor Hunt]

It appears it regressed again; issue is back as of 53.0a1 (2016-11-27) (64-bit OSX).

This time, Cmd+Click still works, but the context menu doesn't recognize the link.

Comment 24

[Daniel Holbert [:dholbert]]

(In reply to tigt from comment #23)
> It appears it regressed again; [...]
> This time, Cmd+Click still works, but the context menu doesn't recognize the link.

Thanks -- actually, it seems that was still a known issue when the fix for this landed -- see comment 9.  Gijs filed bug 1225052 on that.

(So, I don't think anything actually regressed again.  If you think something did, please be more specific about what used to work & now doesn't, & when it previously worked.)

Comment 25

[Taylor Hunt]

You're completely right; I misread. Sorry about that.