Last Comment Bug 480356 - FillInHTMLTooltip should move into XBL binding/equiv
: FillInHTMLTooltip should move into XBL binding/equiv
Status: RESOLVED FIXED
[fixes-834336]
: addon-compat
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla22
Assigned To: Neil Deakin
:
Mentors:
http://mxr.mozilla.org/comm-central/i...
Depends on:
Blocks: 834336 836233 961444
  Show dependency treegraph
 
Reported: 2009-02-26 10:42 PST by Magnus Melin
Modified: 2014-01-21 06:00 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (25.13 KB, patch)
2013-01-31 08:23 PST, Neil Deakin
no flags Details | Diff | Review
Patch v2 (25.19 KB, patch)
2013-02-04 09:22 PST, Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe]
no flags Details | Diff | Review
Updated tests (25.15 KB, patch)
2013-02-07 11:53 PST, Neil Deakin
no flags Details | Diff | Review
Updated patch (25.25 KB, patch)
2013-02-27 06:22 PST, Neil Deakin
neil: review+
mratcliffe: review+
Details | Diff | Review

Description Magnus Melin 2009-02-26 10:42:29 PST
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.
Comment 1 Neil Deakin 2013-01-31 08:23:41 PST
Created attachment 708603 [details] [diff] [review]
Patch

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"/>
Comment 2 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2013-02-04 09:22:53 PST
Created attachment 709753 [details] [diff] [review]
Patch v2

Rebased
Comment 3 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2013-02-04 09:27:31 PST
This fixes bug 834336 ... is this ready for review?
Comment 4 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2013-02-06 01:52:12 PST
Comment on attachment 709753 [details] [diff] [review]
Patch v2

Seems like it is
Comment 5 Neil Deakin 2013-02-06 04:42:52 PST
Comment on attachment 709753 [details] [diff] [review]
Patch v2

It isn't yet.
Comment 6 Neil Deakin 2013-02-07 11:53:02 PST
Created attachment 711445 [details] [diff] [review]
Updated tests

Fixed some errors with some tests. Not sure who should review.
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-02-07 13:18:14 PST
Comment on attachment 711445 [details] [diff] [review]
Updated tests

Perhaps other-Neil can review?
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-02-07 13:20:58 PST
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 9 neil@parkwaycc.co.uk 2013-02-21 06:28:03 PST
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...
Comment 10 Neil Deakin 2013-02-27 06:22:16 PST
Created attachment 718973 [details] [diff] [review]
Updated patch
Comment 11 Neil Deakin 2013-02-27 06:24:13 PST
> >+          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 neil@parkwaycc.co.uk 2013-02-27 07:46:25 PST
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.
Comment 13 Neil Deakin 2013-02-27 08:09:20 PST
I meant I didn't see the references to either of those in the Seamonkey code.
Comment 14 Michael Ratcliffe PTO -> Fri, 24 Jun 2016 [:miker] [:mratcliffe] 2013-02-27 09:08:34 PST
Comment on attachment 718973 [details] [diff] [review]
Updated patch

Looks good to me.
Comment 15 neil@parkwaycc.co.uk 2013-02-27 12:05:12 PST
(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 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-02-28 11:04:01 PST
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?
Comment 17 Paul Rouget [:paul] 2013-03-08 04:19:14 PST
(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?
Comment 18 Neil Deakin 2013-03-08 06:23:08 PST
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 19 neil@parkwaycc.co.uk 2013-03-21 05:26:24 PDT
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;
Comment 21 Ryan VanderMeulen [:RyanVM] 2013-03-21 14:10:08 PDT
https://hg.mozilla.org/mozilla-central/rev/431d49245e29

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