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

VERIFIED FIXED in Firefox 44

Status

()

Firefox
Tabbed Browser
VERIFIED FIXED
2 years ago
7 months ago

People

(Reporter: tigt, Assigned: Gijs)

Tracking

(Blocks: 1 bug, {regression})

43 Branch
Firefox 45
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s?, firefox41 unaffected, firefox42 unaffected, firefox43+ wontfix, firefox44+ verified, firefox45 verified, firefox-esr38 unaffected)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
Created attachment 8677685 [details]
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.
(Reporter)

Comment 1

2 years ago
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

2 years ago
[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
Status: UNCONFIRMED → NEW
status-firefox41: --- → unaffected
status-firefox42: --- → unaffected
status-firefox43: --- → affected
status-firefox44: --- → affected
status-firefox-esr38: --- → unaffected
tracking-firefox43: --- → ?
tracking-firefox44: --- → ?
Ever confirmed: true
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(felipc)
Keywords: regression
(Assignee)

Comment 3

2 years ago
(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?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(alice0775)

Comment 4

2 years ago
(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.
tracking-e10s: --- → ?
Flags: needinfo?(alice0775)
Summary: Right-clicking, Cmd+Clicking doesn't work on links in inline SVG → [e10s] Right-clicking, Cmd+Clicking doesn't work on links in inline SVG
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.
tracking-firefox43: ? → +
tracking-firefox44: ? → +
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.
status-firefox43: affected → wontfix
Component: Untriaged → SVG
Product: Firefox → Core
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?
(Assignee)

Comment 8

2 years ago
Created attachment 8686684 [details]
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
Attachment #8686684 - Flags: review?(felipc)
(Assignee)

Comment 9

2 years ago
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.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Component: SVG → Tabbed Browser
Flags: needinfo?(felipc)
Product: Core → Firefox
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
Attachment #8686684 - Flags: review?(felipc) → review+
(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?
(Assignee)

Comment 12

2 years ago
(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

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

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bece86c18efc
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
(Assignee)

Updated

2 years ago
Blocks: 1225052
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.
Blocks: 1227116
Status: RESOLVED → VERIFIED
Gijs, should we uplift this to Aurora44?
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 17

2 years ago
(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.
(Assignee)

Comment 18

2 years ago
Created attachment 8694715 [details]
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
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8694715 - Flags: approval-mozilla-aurora?
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.
Attachment #8694715 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 20

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/b7632a320e0b
status-firefox44: affected → fixed

Comment 21

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/b7632a320e0b
status-b2g-v2.5: --- → fixed

Comment 22

2 years ago
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]
status-b2g-v2.5: fixed → ---
(Assignee)

Updated

2 years ago
status-firefox44: fixed → verified
status-firefox45: fixed → verified
(Reporter)

Comment 23

7 months ago
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.
(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.)
(Reporter)

Comment 25

7 months ago
You're completely right; I misread. Sorry about that.
You need to log in before you can comment on or make changes to this bug.