The FillInHTMLTooltip function has a comment 2534 * XXX - this must move into XBL binding/equiv! Do not want to pollute 2535 * browser.js with functionality that can be encapsulated into 2536 * browser widget. TEMPORARY! Filing this as a bug instead of having just a comment.

This patch removes all the manual tooltip filling in, and moves this into the tooltip binding. All that is needed to do now is: <tooltip id="pageTooltip" page="true"/> <browser tooltip="pageTooltip"/>

Rebased

This fixes bug 834336 ... is this ready for review?

Comment on attachment 709753 [details] [diff] [review] Patch v2 Seems like it is

Comment on attachment 709753 [details] [diff] [review] Patch v2 It isn't yet.

Fixed some errors with some tests. Not sure who should review.

Comment on attachment 711445 [details] [diff] [review] Updated tests Perhaps other-Neil can review?

Comment on attachment 711445 [details] [diff] [review] Updated tests At least the move-to-toolkit aspects of it, I guess (I imagine SeaMonkey would want an equivalent change). I can try to review the browser-specific parts.

Comment on attachment 711445 [details] [diff] [review] Updated tests Sorry for the delay. >+ <property name="page" >+ onget="return this.getAttribute('page');" >+ onset="this.setAttribute('page', val); return val;"/> What type of property is this? It gets set and read as if it was a boolean, but this pattern is for string properties... >+ return false; While I was here I thought I would compare the SeaMonkey and Firefox versions, which are broadly the same, however this change happens to match SeaMonkey :-) >+ var direction = tipElement.ownerDocument.dir; [SeaMonkey sets the direction of the validation message based on the element; I don't claim that this is correct behaviour.] >+ !tipElement.hasAttribute('title') && [Nit: SeaMonkey uses "title".] >+ while (!titleText && !XLinkTitleText && !SVGTitleText && tipElement) { [SeaMonkey treats title="" as meaning to inhibit the parent title; I don't claim that this is correct behaviour.] >+ for (let childNode of tipElement.childNodes) { [Nice! I should use for ... of more in SeaMonkey.] >+ var defView = tipElement.ownerDocument.defaultView; >+ // XXX Work around bug 350679: >+ // "Tooltips can be fired in documents with no view". >+ if (!defView) >+ return false; [SeaMonkey tests this earlier on in the method.] >+ direction = defView.getComputedStyle(tipElement, "") >+ .getPropertyValue("direction"); [Nit: SeaMonkey lines the .s up.] >+ var retVal = false; >+ var self = this; >+ [titleText, XLinkTitleText, SVGTitleText].forEach(function (t) { [SeaMonkey uses return [...].some(...) instead which might produce slightly different results if the same element has more than one title text?] >+ // Make CRLF and CR render one line break each. >+ t = t.replace(/\r\n?/g, '\n'); >+ >+ self.label = t; [SeaMonkey does this on one line.] Nit: pass this to forEach so that you can use this.label instead of self. >+ <handler event="popupshowing"><![CDATA[ >+ if (this.page && !this.fillInPageTooltip(this.triggerNode)) { >+ event.preventDefault(); >+ } >+ ]]></handler> I wonder whether this should be a child binding...

> >+ var direction = tipElement.ownerDocument.dir; > [SeaMonkey sets the direction of the validation message based on the element; I don't claim that this is correct behaviour.] > > >+ !tipElement.hasAttribute('title') && > [Nit: SeaMonkey uses "title".] I didn't see where either of these were. > I wonder whether this should be a child binding... I didn't so that someone could call fillInPageTooltip manually.

Comment on attachment 718973 [details] [diff] [review] Updated patch >+ var direction = tipElement.ownerDocument.dir; >+ >+ // If the element is invalid per HTML5 Forms specifications and has no title, >+ // show the constraint validation error message. >+ if ((tipElement instanceof HTMLInputElement || >+ tipElement instanceof HTMLTextAreaElement || >+ tipElement instanceof HTMLSelectElement || >+ tipElement instanceof HTMLButtonElement) && >+ !tipElement.hasAttribute('title') && (In reply to Neil Deakin from comment #11) > > >+ var direction = tipElement.ownerDocument.dir; > > [SeaMonkey sets the direction of the validation message based on the element; > > I don't claim that this is correct behaviour.] > > > > >+ !tipElement.hasAttribute('title') && > > [Nit: SeaMonkey uses "title".] > > I didn't see where either of these were.

I meant I didn't see the references to either of those in the Seamonkey code.

Comment on attachment 718973 [details] [diff] [review] Updated patch Looks good to me.

(In reply to Neil Deakin from comment #11) > >+ var direction = tipElement.ownerDocument.dir; > [SeaMonkey sets the direction of the validation message based on the element; I don't claim that this is correct behaviour.] http://mxr.mozilla.org/comm-central/source/suite/common/utilityOverlay.js#1729 > >+ !tipElement.hasAttribute('title') && > [Nit: SeaMonkey uses "title".] http://mxr.mozilla.org/comm-central/source/suite/common/utilityOverlay.js#1738

addon-compat: this removes the FillInHTMLTooltip function from the Firefox window scope (several add-ons appear to call it). If this proves to be a problem we may want to add an empty placeholder function to avoid add-on bustage. Should the comment about DefaultTooltipTextProvider::GetNodeText remain next to the popup.xml version of the function?

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #16) > addon-compat: this removes the FillInHTMLTooltip function from the Firefox > window scope (several add-ons appear to call it). If this proves to be a > problem we may want to add an empty placeholder function to avoid add-on > bustage. > > Should the comment about DefaultTooltipTextProvider::GetNodeText remain next > to the popup.xml version of the function? Neil, what do you think? Can I land that?

If Gavin is happy with the patch with those two changes (it otherwise hasn't been reviewed by a browser peer), then yes, feel free to make those changes and check in.

Comment on attachment 718973 [details] [diff] [review] Updated patch >+ ...function (t) { >+ if (t && /\S/.test(t)) { >+ // Make CRLF and CR render one line break each. >+ this.label = t.replace(/\r\n?/g, '\n'); >+ return true; >+ } >+ }... Just spotted that this anonymous function is missing a return false; i.e. if (t && /\S/.test(t)) { // Make CRLF and CR render one line break each. this.label = t.replace(/\r\n?/g, '\n'); return true; } return false;