FillInHTMLTooltip should move into XBL binding/equiv

RESOLVED FIXED in mozilla22

Status

()

Core
General
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: Magnus Melin, Assigned: Neil Deakin (mostly unavailable until September))

Tracking

({addon-compat})

Trunk
mozilla22
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixes-834336], URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

9 years ago
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.
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"/>
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Blocks: 834336
Created attachment 709753 [details] [diff] [review]
Patch v2

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)
Created attachment 711445 [details] [diff] [review]
Updated tests

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 9

5 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...
Whiteboard: [fixes-834336]
Created attachment 718973 [details] [diff] [review]
Updated patch
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]

Updated

5 years ago
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

Comment 17

5 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]
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/integration/mozilla-inbound/rev/431d49245e29
https://hg.mozilla.org/mozilla-central/rev/431d49245e29
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22

Updated

4 years ago
Blocks: 961444
You need to log in before you can comment on or make changes to this bug.