Last Comment Bug 510202 - xlink:title should only work on links
: xlink:title should only work on links
Status: RESOLVED FIXED
: testcase
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Robert Longson
:
Mentors:
https://bugs.webkit.org/attachment.cg...
Depends on: 332773 async-visited-check
Blocks: xlink-mathml
  Show dependency treegraph
 
Reported: 2009-08-13 07:26 PDT by jonathan chetwynd
Modified: 2010-03-13 03:02 PST (History)
4 users (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (5.87 KB, patch)
2010-02-12 08:12 PST, Robert Longson
no flags Details | Diff | Splinter Review
remove link type code per bz comment (5.65 KB, patch)
2010-02-13 05:37 PST, Robert Longson
dao+bmo: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
updated patch (5.52 KB, patch)
2010-02-21 09:45 PST, Robert Longson
no flags Details | Diff | Splinter Review
post bug 461199 (2.70 KB, patch)
2010-02-21 09:48 PST, Robert Longson
bzbarsky: review+
Details | Diff | Splinter Review
clickable <link>s (844 bytes, text/html; charset=utf-16)
2010-02-23 20:07 PST, Boris Zbarsky [:bz]
no flags Details
updated post bug 461199 patch (5.00 KB, patch)
2010-03-11 04:33 PST, Robert Longson
sdwilsh: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
post bug 46119 with iid rev (5.53 KB, patch)
2010-03-11 14:15 PST, Robert Longson
no flags Details | Diff | Splinter Review

Description jonathan chetwynd 2009-08-13 07:26:09 PDT
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.
Comment 1 jonathan chetwynd 2009-08-13 07:35:02 PDT
neither safari nor opera mimic this behaviour.
Comment 2 Robert Longson 2010-02-12 08:12:55 PST
Created attachment 426679 [details] [diff] [review]
patch
Comment 3 Boris Zbarsky [:bz] 2010-02-12 09:00:14 PST
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?
Comment 4 Robert Longson 2010-02-12 09:13:41 PST
I thought mathml links were just xmlns:type="simple". Is the mathml linking bug 427990?

Erm. Once Shawn lands what?
Comment 5 Robert Longson 2010-02-12 09:15:03 PST
http://www.w3.org/TR/REC-MathML/chapter7.html#sec7.1.5 was what I was looking at.
Comment 6 Boris Zbarsky [:bz] 2010-02-12 09:17:04 PST
> 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.
Comment 7 Robert Longson 2010-02-13 05:37:18 PST
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.
Comment 8 Dão Gottwald [:dao] 2010-02-13 11:16:24 PST
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.
Comment 9 Robert Longson 2010-02-21 09:45:01 PST
Created attachment 428068 [details] [diff] [review]
updated patch
Comment 10 Robert Longson 2010-02-21 09:48:08 PST
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.
Comment 11 Boris Zbarsky [:bz] 2010-02-22 10:47:15 PST
> <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.
Comment 12 Robert Longson 2010-02-22 11:27:10 PST
Comment on attachment 426824 [details] [diff] [review]
remove link type code per bz comment

link version resurrected
Comment 13 Boris Zbarsky [:bz] 2010-02-22 11:45:32 PST
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.
Comment 14 Boris Zbarsky [:bz] 2010-02-22 11:46:01 PST
Comment on attachment 428069 [details] [diff] [review]
post bug 461199

r=bzbarsky
Comment 15 Dão Gottwald [:dao] 2010-02-23 14:39:13 PST
(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.
Comment 16 Robert Longson 2010-02-23 14:40:03 PST
checked in http://hg.mozilla.org/mozilla-central/rev/9d0a036ed2cb
Comment 17 Boris Zbarsky [:bz] 2010-02-23 20:07:38 PST
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.
Comment 18 Robert Longson 2010-02-23 21:36:57 PST
Comment 16 fixed the bug, although I still need to land the follow up once bug 461199 lands.
Comment 19 Robert Longson 2010-03-10 15:54:01 PST
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.
Comment 20 Boris Zbarsky [:bz] 2010-03-10 15:58:26 PST
Hmm.  Shawn, can we expose the URI via some sort of virtual method (possibly in addition to GetURI) so this can work?
Comment 21 Shawn Wilsher :sdwilsh 2010-03-10 16:10:06 PST
(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.
Comment 22 Boris Zbarsky [:bz] 2010-03-10 19:31:15 PST
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.
Comment 23 Shawn Wilsher :sdwilsh 2010-03-10 20:06:51 PST
Why don't we just change the visibility then since that seems simplest:
NS_EXTERNAL_VIS(already_AddRefed<nsIURI>) GetURI() const;
Comment 24 Boris Zbarsky [:bz] 2010-03-10 20:17:49 PST
If that makes things link on all platforms, I'm ok with that, I think.
Comment 25 Karl Tomlinson (:karlt) 2010-03-10 20:22:21 PST
Ideally NS_EXTERNAL_VIS would only be applied in dynamic builds.
Comment 26 Shawn Wilsher :sdwilsh 2010-03-10 22:15:31 PST
(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?
Comment 27 Robert Longson 2010-03-11 04:33:45 PST
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.
Comment 28 Shawn Wilsher :sdwilsh 2010-03-11 13:46:11 PST
Comment on attachment 431837 [details] [diff] [review]
updated post bug 461199 patch

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

r=sdwilsh
Comment 29 Robert Longson 2010-03-11 14:15:38 PST
Created attachment 431971 [details] [diff] [review]
post bug 46119 with iid rev
Comment 30 Robert Longson 2010-03-13 03:02:58 PST
pushed http://hg.mozilla.org/mozilla-central/rev/d07982d3f426

Note You need to log in before you can comment on or make changes to this bug.