Closed
Bug 1429709
Opened 6 years ago
Closed 6 years ago
SVG relative link missing context menu entries
Categories
(Firefox :: Menus, defect, P5)
Tracking
()
RESOLVED
FIXED
Firefox 59
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox57 | --- | wontfix |
firefox58 | --- | wontfix |
firefox59 | --- | fixed |
People
(Reporter: marius.lessiak, Assigned: Gijs)
References
Details
(Keywords: regression)
Attachments
(2 files)
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.132 Safari/537.36 Steps to reproduce: View a svg containing <a> with relative links in href attribute. Right click on link to open context menu. Actual results: Context menu ist missing most entries, amongst them the open in new tab / open in new window / open in private window. Expected results: Context menu with the correct set of entries should be displayed. As it is for an absolute link.
Comment 1•6 years ago
|
||
I confirm this bug on latest nightly on windows 7
Status: UNCONFIRMED → NEW
Component: Untriaged → Menus
Ever confirmed: true
Comment 2•6 years ago
|
||
This is because linkProtocol is null at https://searchfox.org/mozilla-central/rev/88439dc6c5b02e167032374071cdc697a056efa1/browser/modules/ContextMenu.jsm#331 in this case. The absolute link has a linkProtocol of "http". We only show the "open link in new tab"-menu items on "saveable links", but we show the "copy link location" item on all links with the href attribute. I'm not sure why we only show those options for "saveable links" so I did a bit of code spelunking and found that it has been this way since at least 2002 from this commit, https://searchfox.org/mozilla-central/commit/54083b0fac37b04d25839c459c9ca117df8df9fa I wasn't able to find anything from before then or Blake's reference to the old nsContextMenu.js.
Priority: -- → P5
Comment 3•6 years ago
|
||
Actually, this was broken by a different changeset, https://searchfox.org/mozilla-central/diff/ea950f6b1dd997a3b3f8db6603c17ed6716d70dd/browser/base/content/nsContextMenu.js#1684 We aren't making SVG links absolute.
Assignee | ||
Comment 4•6 years ago
|
||
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3) > Actually, this was broken by a different changeset, > https://searchfox.org/mozilla-central/diff/ > ea950f6b1dd997a3b3f8db6603c17ed6716d70dd/browser/base/content/nsContextMenu. > js#1684 > > We aren't making SVG links absolute. Gahhh. SVG makes hard everything that the DOM is supposed to make easy. Like, that code reads fine. The only issue is that apparently getting `.animVal` on the SVGOM doesn't absolutize the `href` attribute (or check whether it's even valid!). Which actually seems like a bug. If it's per spec, I would strongly argue it's a spec bug (then again, I would also argue that .href being an object "just" for xlink:href SVG is a spec bug - but I digress). :bbirtles/longsonr, is there a sane way of getting this information out of the DOM directly, or do we need to do all the hard work of constructing a URL ourselves?
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(longsonr)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(bbirtles)
Assignee | ||
Updated•6 years ago
|
status-firefox57:
--- → wontfix
status-firefox58:
--- → wontfix
status-firefox59:
--- → affected
status-firefox-esr52:
--- → unaffected
Comment 5•6 years ago
|
||
Looking at the spec, there's nothing there that requires absolutizing the href. Looking at the implementation, I notice we have SVGAElement::GetHrefURI which returns an nsIURI with suitable base URI etc. so I wonder if there's some way to get to the result of that from JS. If not I guess we could add a chrome-only API for that until we spec something. (Unfortunately the SVG DOM is a real mess. We want to drop animVal altogether but it's not clear how to handle legacy content. I think the current proposal is just to alias animVal to baseVal but that still doesn't make `elem.href` behave like a string.) CC'ing Cameron in case he knows of any other way of getting an absolute href out of an SVG <a> from JS.
Flags: needinfo?(bbirtles)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•6 years ago
|
||
Looks like there's already a context menu helper, so might as well just use that. Less work than exposing this through webidl (though tbf, that is also not super hard to do).
Comment 8•6 years ago
|
||
You'd need to do the hard work yourselves. It's complicated because of SMIL. .animVal and href may be different e.g. <a> <set attributeName="href" to="https://www.google.com" begin="0s" dur="2s" fill="freeze" /> </a> will cause the .animVal property to return https://www.google.com and the link to work to send you to google but hasAttribute will be false and getAttribute won't return an attribute.
Flags: needinfo?(longsonr)
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8942018 [details] Bug 1429709 - make relative SVG links absolute, https://reviewboard.mozilla.org/r/212220/#review218098
Attachment #8942018 -
Flags: review?(jaws) → review+
Comment 10•6 years ago
|
||
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/e50d4cdb9fa7 make relative SVG links absolute, r=jaws
Comment 11•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e50d4cdb9fa7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in
before you can comment on or make changes to this bug.
Description
•