Closed Bug 480356 Opened 15 years ago Closed 11 years ago

FillInHTMLTooltip should move into XBL binding/equiv

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: mkmelin, Assigned: enndeakin)

References

()

Details

(Keywords: addon-compat, Whiteboard: [fixes-834336])

Attachments

(1 file, 3 obsolete files)

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.
Attached patch Patch (obsolete) — Splinter Review
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
Attached patch Patch v2 (obsolete) — Splinter Review
Rebased
Attachment #708603 - Attachment is obsolete: true
This fixes bug 834336 ... is this ready for review?
Comment on attachment 709753 [details] [diff] [review]
Patch v2

Seems like it is
Attachment #709753 - Flags: review?(gavin.sharp)
Comment on attachment 709753 [details] [diff] [review]
Patch v2

It isn't yet.
Attachment #709753 - Flags: review?(gavin.sharp)
Attached patch Updated tests (obsolete) — Splinter Review
Fixed some errors with some tests. Not sure who should review.
Attachment #709753 - Attachment is obsolete: true
Attachment #711445 - Flags: review?(gavin.sharp)
Comment on attachment 711445 [details] [diff] [review]
Updated tests

Perhaps other-Neil can review?
Attachment #711445 - Flags: review?(gavin.sharp) → review?(neil)
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 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...
Whiteboard: [fixes-834336]
Attached patch Updated patchSplinter Review
Attachment #711445 - Attachment is obsolete: true
Attachment #711445 - Flags: review?(neil)
Attachment #711445 - Flags: feedback?(gavin.sharp)
Attachment #718973 - Flags: review?(neil)
> >+          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.
Attachment #718973 - Flags: review?(neil) → review+
I meant I didn't see the references to either of those in the Seamonkey code.
Attachment #718973 - Flags: review?(mratcliffe)
Comment on attachment 718973 [details] [diff] [review]
Updated patch

Looks good to me.
Attachment #718973 - Flags: review?(mratcliffe) → review+
Whiteboard: [fixes-834336] → [fixes-834336][land-in-fx-team]
Blocks: 836233
(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?
Keywords: addon-compat
(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]
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 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;
https://hg.mozilla.org/mozilla-central/rev/431d49245e29
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Blocks: 961444
You need to log in before you can comment on or make changes to this bug.