xlink:title should only work on links

RESOLVED FIXED

Status

()

Firefox
General
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jonathan chetwynd, Assigned: Robert Longson)

Tracking

({testcase})

Trunk
testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

8 years ago
mouse over the orange box.
a tooltip is raised, 
NB this is not an anchor.

Is this a bug?
SVG WG discussion:
http://lists.w3.org/Archives/Public/www-svg/2009Aug/0030.html

one proposed use case is: onEvent script is used to import data using xmlhttprequest

for this purpose title is (may?) not appropriate,
ie similar issue to alt and title in html,

title describes image, 
xlink:title describes linked resource.
(Reporter)

Comment 1

8 years ago
neither safari nor opera mimic this behaviour.
OS: Mac OS X → All
Hardware: PowerPC → All
(Assignee)

Updated

8 years ago
Component: SVG → General
Product: Core → Firefox
QA Contact: general → general
(Assignee)

Comment 2

8 years ago
Created attachment 426679 [details] [diff] [review]
patch
Assignee: nobody → longsonr
Attachment #426679 - Flags: review?(dao)
Note that the xlink:type thing is probably unnecessary and that this list would need to be expanded when we fix the mathml linking bug.  Or something.

In the C++ code, can we just QI to Link (at least once Shawn lands) instead of all the hardcoding?
(Assignee)

Comment 4

8 years ago
I thought mathml links were just xmlns:type="simple". Is the mathml linking bug 427990?

Erm. Once Shawn lands what?
(Assignee)

Comment 5

8 years ago
http://www.w3.org/TR/REC-MathML/chapter7.html#sec7.1.5 was what I was looking at.
> I thought mathml links were just xmlns:type="simple".

They are, but not all xlink:type="simple" things are links.

The Mathml linking bug is indeed bug 427990.

Shawn is hopefully going to land bug 461199 and appurtenances thereunto today.
(Assignee)

Comment 7

8 years ago
Created attachment 426824 [details] [diff] [review]
remove link type code per bz comment

We can put a note in bug 427990 about changing this code when writing a patch for that.

If bug 461199 lands I can rework the C++ code, if not I can raise a follow up and fix it once it does.
Attachment #426679 - Attachment is obsolete: true
Attachment #426679 - Flags: review?(dao)
Attachment #426824 - Flags: review?(dao)
Comment on attachment 426824 [details] [diff] [review]
remove link type code per bz comment

>+      if ((tipElement instanceof HTMLAnchorElement && tipElement.href) ||
>+          (tipElement instanceof HTMLAreaElement && tipElement.href) ||
>+          tipElement instanceof HTMLLinkElement ||

I don't think the HTMLLinkElement line makes sense. <link> is always empty and only permitted in <head>, as far as I know... hence no way to invoke a tooltip.

You'll need somebody else to review the embedding/ part.
Attachment #426824 - Flags: review?(dao) → review+
(Assignee)

Comment 9

8 years ago
Created attachment 428068 [details] [diff] [review]
updated patch
Attachment #426824 - Attachment is obsolete: true
Attachment #428068 - Flags: review?(bzbarsky)
(Assignee)

Comment 10

8 years ago
Created attachment 428069 [details] [diff] [review]
post bug 461199

I can either make the change so it looks like this once bug 461199 lands or alternatively ask Shawn to include this change when he lands bug 461199. Assuming it's right of course.
Attachment #428069 - Flags: review?(bzbarsky)
(Assignee)

Updated

8 years ago
Attachment #428068 - Attachment is patch: true
> <link> is always empty

Not necessarily.

> hence no way to invoke a tooltip.

Would you like a testcase that makes an <html:link> not only hoverable but also clickable as a link?

The updated patches look ok to me except for the <link> issue and the fact that they will treat some things as links that aren't actually links (cases where the href attribute is set but can't be used to create an nsIURI).  Not sure there's much you can do about the latter, but please fix the <link> thing.
(Assignee)

Updated

8 years ago
Attachment #426824 - Attachment is obsolete: false
Attachment #426824 - Flags: superreview?(bzbarsky)
(Assignee)

Comment 12

8 years ago
Comment on attachment 426824 [details] [diff] [review]
remove link type code per bz comment

link version resurrected
(Assignee)

Updated

8 years ago
Attachment #428068 - Flags: review?(bzbarsky)
Comment on attachment 426824 [details] [diff] [review]
remove link type code per bz comment

You want to check @href for <link> as well, probably.  sr=me with that.
Attachment #426824 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 428069 [details] [diff] [review]
post bug 461199

r=bzbarsky
Attachment #428069 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

8 years ago
Attachment #428068 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Summary: xlink is anchor only? → xlink:title should only work on links
(In reply to comment #11)
> > <link> is always empty
> 
> Not necessarily.
> 
> > hence no way to invoke a tooltip.
> 
> Would you like a testcase that makes an <html:link> not only hoverable but also
> clickable as a link?

A testcase would be interesting, since my reading of the html spec seemed to support my naive understanding.
(Assignee)

Comment 16

8 years ago
checked in http://hg.mozilla.org/mozilla-central/rev/9d0a036ed2cb
Created attachment 428620 [details]
clickable <link>s

> A testcase would be interesting

Here you go.  Note that the <link>s are visible in webkit and opera too, but they don't make them into actual clickable links.  We do.  If we stop doing that, we can stop treating them as links here too...

Also note that I could have used kids appended to the <link>s via the DOM or an XML testcase instead of CSS generated content.
(Assignee)

Comment 18

8 years ago
Comment 16 fixed the bug, although I still need to land the follow up once bug 461199 lands.
Blocks: 427990
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Depends on: 461199
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Flags: in-testsuite+
Attachment #428620 - Attachment mime type: text/html → text/html; charset=utf-16
(Assignee)

Comment 19

8 years ago
The follow up to use mozilla::dom::Link breaks non-libxul builds.

            nsCOMPtr<mozilla::dom::Link> linkContent(do_QueryInterface(currElement));
            if (linkContent) {
              nsCOMPtr<nsIURI> uri(linkContent->GetURI());

results in the linker complaining that GetURI can't be found. I don't know what to do about that since adding 

SHARED_LIBRARY_LIBS = \
		$(DEPTH)/content/base/src/$(LIB_PREFIX)gkconbase_s.$(LIB_SUFFIX) \
		$(NULL)

to the Makefile results in 1 link error becoming 238 link errors from the dependent libraries that gkconbase_s requires.
Hmm.  Shawn, can we expose the URI via some sort of virtual method (possibly in addition to GetURI) so this can work?
(In reply to comment #20)
> Hmm.  Shawn, can we expose the URI via some sort of virtual method (possibly in
> addition to GetURI) so this can work?
There should be some way to make linking work here, shouldn't we?  If push comes to shove, we can always add some new interface I guess.
The ways to make that linking work are to either inline the method, make the two things part of the same library, make it virtual (or add a GetURIExternal method that's virtual), or change visibility so it's globally exposed from the module...  Usually in this situation we add a virtual *External method.
Why don't we just change the visibility then since that seems simplest:
NS_EXTERNAL_VIS(already_AddRefed<nsIURI>) GetURI() const;
If that makes things link on all platforms, I'm ok with that, I think.
Ideally NS_EXTERNAL_VIS would only be applied in dynamic builds.
(In reply to comment #25)
> Ideally NS_EXTERNAL_VIS would only be applied in dynamic builds.
Yeah, maybe we could make a new define for that?
(Assignee)

Comment 27

8 years ago
Created attachment 431837 [details] [diff] [review]
updated post bug 461199 patch

I can't get NS_EXTERNAL_VIS to link. virtual works a treat though.
Attachment #428069 - Attachment is obsolete: true
Attachment #431837 - Flags: superreview?(bzbarsky)
Attachment #431837 - Flags: review?(sdwilsh)
Comment on attachment 431837 [details] [diff] [review]
updated post bug 461199 patch

you'll need to rev the Link CID too, right?

r=sdwilsh
Attachment #431837 - Flags: review?(sdwilsh) → review+
Attachment #431837 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 29

8 years ago
Created attachment 431971 [details] [diff] [review]
post bug 46119 with iid rev
Attachment #431837 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #431971 - Attachment description: port bug 46119 with iid rev → post bug 46119 with iid rev
(Assignee)

Comment 30

8 years ago
pushed http://hg.mozilla.org/mozilla-central/rev/d07982d3f426
You need to log in before you can comment on or make changes to this bug.