Closed Bug 510202 Opened 15 years ago Closed 14 years ago

xlink:title should only work on links

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jay, Assigned: longsonr)

References

(
URL
)

Details

(Keywords: testcase)

Attachments

(3 files, 4 obsolete files)

remove link type code per bz comment
5.65 KB, patch
dao
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
clickable <link>s
844 bytes, text/html; charset=utf-16
Details
post bug 46119 with iid rev
5.53 KB, patch
Details | Diff | Splinter Review 
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.
neither safari nor opera mimic this behaviour.
OS: Mac OS X → All
Hardware: PowerPC → All
Component: SVG → General
Product: Core → Firefox
QA Contact: general → general
Attached patch patch (obsolete) — — Splinter Review 
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?

    
    

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

Erm. Once Shawn lands what?

    
    

    
  
> 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.

    
    

    
  


  
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 #426824 -
        Flags: review?(dao)

        Attachment #426679 -
        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+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        updated patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #426824 -
        Attachment is obsolete: true

        Attachment #428068 -
        Flags: review?(bzbarsky)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        post bug 461199 (obsolete)
        — 
          — Splinter Review
      

    


  
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)

    
  

        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.

    
  

        Attachment #426824 -
        Attachment is obsolete: false

        Attachment #426824 -
        Flags: superreview?(bzbarsky)

    
    

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

link version resurrected

    
  

        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+

    
  

        Attachment #428068 -
        Attachment is obsolete: true

    
  
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.

    
    

    
  

      
      
      
      

        Attached file
          
        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 16 fixed the bug, although I still need to land the follow up once bug 461199 lands.
Blocks: xlink-mathml
Status: NEW → RESOLVED
Closed: 14 years ago
Depends on: async-visited-check
Resolution: --- → FIXED

    
  
Flags: in-testsuite+

        Attachment #428620 -
        Attachment mime type: text/html → text/html; charset=utf-16

    
    

    
  
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?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        updated post bug 461199 patch (obsolete)
        — 
          — Splinter Review
      

    


  
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+

    
    

    
  


  

        Attachment #431837 -
        Attachment is obsolete: true

    
  

        Attachment #431971 -
        Attachment description: port bug 46119 with iid rev → post bug 46119 with iid rev

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: