Closed
Bug 480356
Opened 15 years ago
Closed 11 years ago
FillInHTMLTooltip should move into XBL binding/equiv
Categories
(Core :: General, defect)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: mkmelin, Assigned: enndeakin)
References
()
Details
(Keywords: addon-compat, Whiteboard: [fixes-834336])
Attachments
(1 file, 3 obsolete files)
25.25 KB,
patch
|
neil
:
review+
miker
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
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"/>
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Comment 3•11 years ago
|
||
This fixes bug 834336 ... is this ready for review?
Comment 4•11 years ago
|
||
Comment on attachment 709753 [details] [diff] [review] Patch v2 Seems like it is
Attachment #709753 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 5•11 years ago
|
||
Comment on attachment 709753 [details] [diff] [review] Patch v2 It isn't yet.
Attachment #709753 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 6•11 years ago
|
||
Fixed some errors with some tests. Not sure who should review.
Attachment #709753 -
Attachment is obsolete: true
Attachment #711445 -
Flags: review?(gavin.sharp)
Comment 7•11 years ago
|
||
Comment on attachment 711445 [details] [diff] [review] Updated tests Perhaps other-Neil can review?
Attachment #711445 -
Flags: review?(gavin.sharp) → review?(neil)
Comment 8•11 years ago
|
||
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.
Attachment #711445 -
Flags: feedback?(gavin.sharp)
Comment 9•11 years ago
|
||
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...
Updated•11 years ago
|
Whiteboard: [fixes-834336]
Assignee | ||
Comment 10•11 years ago
|
||
Attachment #711445 -
Attachment is obsolete: true
Attachment #711445 -
Flags: review?(neil)
Attachment #711445 -
Flags: feedback?(gavin.sharp)
Attachment #718973 -
Flags: review?(neil)
Assignee | ||
Comment 11•11 years ago
|
||
> >+ 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 12•11 years ago
|
||
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.
Attachment #718973 -
Flags: review?(neil) → review+
Assignee | ||
Comment 13•11 years ago
|
||
I meant I didn't see the references to either of those in the Seamonkey code.
Assignee | ||
Updated•11 years ago
|
Attachment #718973 -
Flags: review?(mratcliffe)
Comment 14•11 years ago
|
||
Comment on attachment 718973 [details] [diff] [review] Updated patch Looks good to me.
Attachment #718973 -
Flags: review?(mratcliffe) → review+
Updated•11 years ago
|
Whiteboard: [fixes-834336] → [fixes-834336][land-in-fx-team]
Comment 15•11 years ago
|
||
(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
Comment 16•11 years ago
|
||
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?
Keywords: addon-compat
Comment 17•11 years ago
|
||
(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?
Flags: needinfo?(enndeakin)
Whiteboard: [fixes-834336][land-in-fx-team] → [fixes-834336]
Assignee | ||
Comment 18•11 years ago
|
||
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.
Flags: needinfo?(enndeakin)
Comment 19•11 years ago
|
||
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;
Assignee | ||
Comment 20•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/431d49245e29
Comment 21•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/431d49245e29
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in
before you can comment on or make changes to this bug.
Description
•