Last Comment Bug 601091 - Port FillInHTMLTooltip changes from Firefox (e.g. HTML5 form validation)
: Port FillInHTMLTooltip changes from Firefox (e.g. HTML5 form validation)
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.13
Assigned To: Ian Neal
:
Mentors:
Depends on: 329212 358452 581947 595922 599745 605277 606491 639945 655747 693551 715999 738233 745712
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-01 03:52 PDT by Bruno 'Aqualon' Escherl
Modified: 2012-08-01 20:37 PDT (History)
4 users (show)
iann_bugzilla: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
port FillInHTMLTooltip from Firefox and further invalid formular test from bug 581947 (7.69 KB, patch)
2010-10-01 03:52 PDT, Bruno 'Aqualon' Escherl
no flags Details | Diff | Review
port changes from Firefox for FillInHTMLTooltip and tests for bug 581947 and bug 329212 (11.85 KB, patch)
2012-05-20 04:34 PDT, Ian Neal
no flags Details | Diff | Review
moved defView patch (12.04 KB, patch)
2012-05-20 15:31 PDT, Ian Neal
bugzillamozillaorg_serge_20140323: review+
Details | Diff | Review
named listener functions patch (12.69 KB, patch)
2012-05-21 15:41 PDT, Ian Neal
philip.chee: feedback+
Details | Diff | Review
Double instead of single quotes [Checked in: Comment 18] (12.69 KB, patch)
2012-06-03 09:56 PDT, Ian Neal
neil: review+
iann_bugzilla: feedback+
Details | Diff | Review

Description Bruno 'Aqualon' Escherl 2010-10-01 03:52:36 PDT
Created attachment 480070 [details] [diff] [review]
port FillInHTMLTooltip from Firefox and further invalid formular test from bug 581947

The patch just copies the current FillInHTMLTooltip function from Firefox, which is enough to make browser_bug581947.js pass (requires the patch from bug 599745). I still have to find out if the changes in FillInHTMLTooltip need some other ports too. So just requesting feedback? so far, if somebody has the time to look at it.
Comment 1 Philip Chee 2010-11-08 07:22:32 PST
Comment on attachment 480070 [details] [diff] [review]
port FillInHTMLTooltip from Firefox and further invalid formular test from bug 581947

># HG changeset patch
># User Bruno 'Aqualon' Escherl <bruno@escherl.net>
># Date 1285929839 -7200
># Node ID 7d45da13dd6ca3ad786ab62e5f898c5a1067e360
># Parent  97d7af4a0294d432866ede7db3f6740c6d5574ee
>port FillInHTMLTooltip from Firefox and further invalid formular test from bug 581947
>
>diff --git a/suite/browser/test/Makefile.in b/suite/browser/test/Makefile.in
>--- a/suite/browser/test/Makefile.in
>+++ b/suite/browser/test/Makefile.in
>@@ -55,16 +55,17 @@ _TEST_FILES =	test_feed_discovery.html \
> 		bug364677-data.xml \
> 		bug364677-data.xml^headers^ \
> 		$(NULL)
> 
> _BROWSER_FILES = browser_bug413915.js \
>                  browser_bug427559.js \
>                  browser_bug561636.js \
>                  browser_bug562649.js \
>+                 browser_bug581947.js \
>                  browser_bug585511.js \
>                  browser_bug595507.js \
>                  browser_fayt.js \
>                  browser_page_style_menu.js \
>                  page_style_sample.html \
>                  browser_feed_tab.js \
>                  feed_tab.html \
>                  browser_pluginnotification.js \
>diff --git a/suite/browser/test/browser/browser_bug581947.js b/suite/browser/test/browser/browser_bug581947.js
>new file mode 100644
>--- /dev/null
>+++ b/suite/browser/test/browser/browser_bug581947.js
>@@ -0,0 +1,85 @@
>+function check(aElementName, aBarred, aType) {
>+  let doc = gBrowser.contentDocument;
>+  let tooltip = document.getElementById("aHTMLTooltip");
>+  let content = doc.getElementById('content');
>+
>+  let e = doc.createElement(aElementName);
>+  content.appendChild(e);
>+
>+  if (aType) {
>+    e.type = aType;
>+  }
>+
>+  ok(!FillInHTMLTooltip(e),
>+     "No tooltip should be shown when the element is valid");
>+
>+  e.setCustomValidity('foo');
>+  if (aBarred) {
>+    ok(!FillInHTMLTooltip(e),
>+       "No tooltip should be shown when the element is barred from constraint validation");
>+  } else {
>+    ok(FillInHTMLTooltip(e),
>+       "A tooltip should be shown when the element isn't valid");
>+  }
>+
>+  content.removeChild(e);
>+}
>+
>+function todo_check(aElementName, aBarred) {
>+  let doc = gBrowser.contentDocument;
>+  let tooltip = document.getElementById("aHTMLTooltip");
>+  let content = doc.getElementById('content');
>+
>+  let e = doc.createElement(aElementName);
>+  content.appendChild(e);
>+
>+  let cought = false;
>+  try {
>+    e.setCustomValidity('foo');
>+  } catch (e) {
>+    cought = true;
>+  }
>+
>+  todo(!cought, "setCustomValidity should exist for " + aElementName);
>+
>+  content.removeChild(e);
>+}
>+
>+function test () {
>+  waitForExplicitFinish();
>+  gBrowser.selectedTab = gBrowser.addTab();
>+  gBrowser.selectedBrowser.addEventListener("load", function () {
>+    gBrowser.selectedBrowser.removeEventListener("load", arguments.callee, true);
>+
>+    let testData = [
>+    /* element name, barred, type */
>+      [ 'input',    false, null],
>+      [ 'textarea', false, null],
>+      [ 'button',   true,  'button'],
>+      [ 'button',   false, 'submit' ],
>+      [ 'select',   false, null],
>+      [ 'output',   true,  null],
>+      [ 'fieldset', true,  null],
>+      [ 'object', 'false' ],
>+    ];
>+
>+    for each (let data in testData) {
>+      check(data[0], data[1], data[2]);
>+    }
>+
>+    let todo_testData = [
>+      [ 'keygen', 'false' ],
>+    ];
>+
>+    for each(let data in todo_testData) {
>+      todo_check(data[0], data[1]);
>+    }
>+
>+    gBrowser.removeCurrentTab();
>+    finish();
>+  }, true);
>+
>+  content.location = 
>+    "data:text/html,<!DOCTYPE html><html><body><div id='content'></div></body></html>";
>+}
>+
>diff --git a/suite/common/utilityOverlay.js b/suite/common/utilityOverlay.js
>--- a/suite/common/utilityOverlay.js
>+++ b/suite/common/utilityOverlay.js
>@@ -1427,31 +1427,92 @@ function subscribeToFeedMiddleClick(href
>     this.subscribeToFeed(href, event);
>     // unlike for command events, we have to close the menus manually
>     closeMenus(event.target);
>   }
> }
> 
> function FillInHTMLTooltip(tipElement)
> {
>-  if (tipElement.namespaceURI == "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul")
>-    return false;
>+  var retVal = false;
>+  // Don't show the tooltip if the tooltip node is a XUL element or a document.
>+  if (tipElement.namespaceURI == "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" ||
>+      !tipElement.ownerDocument)
>+    return retVal;
> 
>-  while (tipElement instanceof Element) {
>-    if (tipElement.hasAttribute("title")) {
>+  const XLinkNS = "http://www.w3.org/1999/xlink";
>+
>+
>+  var titleText = null;
>+  var XLinkTitleText = null;
>+  var SVGTitleText = null;
>+  var lookingForSVGTitle = true;
>+  var direction = tipElement.ownerDocument.dir;
>+
>+  // If the element is invalid per HTML5 Forms specifications,
>+  // show the constraint validation error message instead of @tooltip.
>+  if (tipElement instanceof HTMLInputElement ||
>+      tipElement instanceof HTMLTextAreaElement ||
>+      tipElement instanceof HTMLSelectElement ||
>+      tipElement instanceof HTMLButtonElement) {
>+    // If the element is barred from constraint validation or valid,
>+    // the validation message will be the empty string.
>+    titleText = tipElement.validationMessage;
>+  }
>+
>+  while (!titleText && !XLinkTitleText && !SVGTitleText && tipElement) {
>+    if (tipElement.nodeType == Node.ELEMENT_NODE) {
>+      titleText = tipElement.getAttribute("title");
>+      if ((tipElement instanceof HTMLAnchorElement && tipElement.href) ||
>+          (tipElement instanceof HTMLAreaElement && tipElement.href) ||
>+          (tipElement instanceof HTMLLinkElement && tipElement.href)
>+          || (tipElement instanceof SVGAElement && tipElement.hasAttributeNS(XLinkNS, "href"))
>+          ) {
>+        XLinkTitleText = tipElement.getAttributeNS(XLinkNS, "title");
>+      }
>+      if (lookingForSVGTitle &&
>+          !(tipElement instanceof SVGElement &&
>+            tipElement.parentNode instanceof SVGElement &&
>+            !(tipElement.parentNode instanceof SVGForeignObjectElement))) {
>+        lookingForSVGTitle = false;
>+      }
>+      if (lookingForSVGTitle) {
>+        let length = tipElement.childNodes.length;
>+        for (let i = 0; i < length; i++) {
>+          let childNode = tipElement.childNodes[i];
>+          if (childNode instanceof SVGTitleElement) {
>+            SVGTitleText = childNode.textContent;
>+            break;
>+          }
>+        }
>+      }
>       var defView = tipElement.ownerDocument.defaultView;
>-      var titleText = tipElement.getAttribute("title");
>       // XXX Work around bug 350679:
>       // "Tooltips can be fired in documents with no view".
>-      if (!defView || !titleText)
>-        return false;
>-
>-      var tipNode = document.getElementById("aHTMLTooltip");
>-      tipNode.style.direction = defView.getComputedStyle(tipElement, "")
>-                                       .getPropertyValue("direction");
>-      tipNode.label = titleText;
>-      return true;
>+      if (!defView)
>+        return retVal;
>+      direction = defView.getComputedStyle(tipElement, "")
>+        .getPropertyValue("direction");
>     }
>     tipElement = tipElement.parentNode;
>   }
> 
>-  return false;
>+  var tipNode = document.getElementById("aHTMLTooltip");
>+  tipNode.style.direction = direction;
>+
>+  [titleText, XLinkTitleText, SVGTitleText].forEach(function (t) {
>+    if (t && /\S/.test(t)) {
>+
>+      // Per HTML 4.01 6.2 (CDATA section), literal CRs and tabs should be
>+      // replaced with spaces, and LFs should be removed entirely.
>+      // XXX Bug 322270: We don't preserve the result of entities like &#13;,
>+      // which should result in a line break in the tooltip, because we can't
>+      // distinguish that from a literal character in the source by this point.
>+      t = t.replace(/[\r\t]/g, ' ');
>+      t = t.replace(/\n/g, '');
>+
>+      tipNode.setAttribute("label", t);
>+      retVal = true;
>+    }
>+  });
>+
>+  return retVal;
> }
Comment 2 Philip Chee 2010-11-15 20:27:55 PST
q.v. Bug 605277 - Do not show the error message in the tooltip if title is set
Comment 3 neil@parkwaycc.co.uk 2010-11-29 05:09:21 PST
Comment on attachment 480070 [details] [diff] [review]
port FillInHTMLTooltip from Firefox and further invalid formular test from bug 581947

I'm not particularly keen on the way this is written, although obviously the current utilityOverlay code can't be readily adapted to work with validation, XLink, SVG etc.

>+      if ((tipElement instanceof HTMLAnchorElement && tipElement.href) ||
>+          (tipElement instanceof HTMLAreaElement && tipElement.href) ||
>+          (tipElement instanceof HTMLLinkElement && tipElement.href)
>+          || (tipElement instanceof SVGAElement && tipElement.hasAttributeNS(XLinkNS, "href"))
>+          ) {
Weird wrapping.

>+      t = t.replace(/[\r\t]/g, ' ');
>+      t = t.replace(/\n/g, '');
I don't want to do this at all.
Comment 4 Philip Chee 2010-11-29 08:13:44 PST
>>+      t = t.replace(/[\r\t]/g, ' ');
>>+      t = t.replace(/\n/g, '');
> I don't want to do this at all.

Yeah, Firefox is doing it wrong, the HTML5 spec made some changes to linebreaking behaviour:

<http://dev.w3.org/html5/spec/rendering.html#the-title-attribute-0>
<http://www.w3.org/TR/html5/elements.html#the-title-attribute>

"U+000A LINE FEED (LF) characters are expected to cause line breaks in the tooltip."
Comment 5 Philip Chee 2010-11-29 08:18:12 PST
> +      tipNode.setAttribute("label", t);
Hmm. I thought that if we want the text to wrap we should be using .textContent?
Comment 6 Philip Chee 2011-05-10 04:11:36 PDT
q.v. Toolkit Bug 655747 - Add BrowserUtils.jsm for commonly used browser-related code
Comment 7 Jens Hatlak (:InvisibleSmiley) 2011-12-23 14:28:39 PST
Looks like this bug is stalled. :-(

I'm particularly interested in getting HTML5 form validation info tooltips, e.g. <input type="email">: Currently you only see that there is a problem (red halo around the input field), but it's only when you try to submit that the browser tells you what the actual problem is. With FF, you can just hover the input field and read the reason from the tooltip. 

If we ripped that part out (maybe give it its own bug and of course taking care that we port bug 605277, too), would that increase the chance of a positive review?
Comment 8 Philip Chee 2012-01-25 21:03:07 PST
>>>+      t = t.replace(/[\r\t]/g, ' ');
>>>+      t = t.replace(/\n/g, '');
>> I don't want to do this at all.
> 
> Yeah, Firefox is doing it wrong, the HTML5 spec made some changes to linebreaking behaviour:
> 
> <http://dev.w3.org/html5/spec/rendering.html#the-title-attribute-0>
> <http://www.w3.org/TR/html5/elements.html#the-title-attribute>
> 
> "U+000A LINE FEED (LF) characters are expected to cause line breaks in the tooltip."

Bug 358452 fixed this for Firefox.

Also see Bug 721142 - Ensure tooltip tabstops are correct
Comment 9 Jens Hatlak (:InvisibleSmiley) 2012-01-26 00:10:42 PST
Aqualon, are you still working on this?

(In reply to Philip Chee from comment #8)
> Bug 358452 fixed this for Firefox.

FTR, the new code is just:

      // Make CRLF and CR render one line break each.  
      t = t.replace(/\r\n?/g, '\n');

But the patch over there adapted tests, too.
Comment 10 Ian Neal 2012-05-20 04:34:35 PDT
Created attachment 625484 [details] [diff] [review]
port changes from Firefox for FillInHTMLTooltip and tests for bug 581947 and bug 329212

This patch:
* Removes bitrot from old patch
* Also ports:
** Bug 358452 - Make the behavior of space, tab, carriage return and line feed in title attribute tooltips HTML5-compliant / IE-compatible
** Bug 595922 - tooltips should not show form validation message if the form element has @novalidate
** Bug 605277 - Do not show the form validation error message in the tooltip if title is set
** Bug 606491 (2/2) - Update test
** Bug 639945 - tooltips are not displayed on inline svg elements
** Bug 693551 - Old tooltip appears when the SVG object that the mouse is over is removed
** Bug 715999 - Old tooltip appears when the SVG object that the mouse is over is removed
** Bug 745712 - FillInHTMLTooltip should not use title attributes from XUL ascendants
* Added test for bug 329212 up to and including Bug 738233 - Fix misplaced brackets in browser_bug329212.js

I have checked that no mochitest-browser-chrome tests fail in suite/browser/test/

Requesting r from Serge for the tests, r from Neil for the non-test changes and general feedback from Serge and Ratty.
Comment 11 neil@parkwaycc.co.uk 2012-05-20 10:20:24 PDT
Comment on attachment 625484 [details] [diff] [review]
port changes from Firefox for FillInHTMLTooltip and tests for bug 581947 and bug 329212

>+  var direction = tipElement.ownerDocument.dir;
Is this the right thing to do for a form validation tooltip? (For other tooltips we pick up the computed direction in the loop.)

>+  while (!titleText && !XLinkTitleText && !SVGTitleText && tipElement) {
This breaks using title="" to disinherit title text. Comparing against null should work though.

>       var defView = tipElement.ownerDocument.defaultView;
Move this (and the comment and check) to near the beginning of the function.

>+  [titleText, XLinkTitleText, SVGTitleText].forEach(function (t) {
>+    if (t && /\S/.test(t)) {
>+      // Make CRLF and CR render one line break each.
>+      t = t.replace(/\r\n?/g, '\n');
>+
>+      tipNode.setAttribute("label", t);
>+      retVal = true;
>+    }
>+  });
>+  return retVal;
Might be possible to write this using .some() i.e.
return [titleText, XLinkTitleText, SVGTitleText].some(function (t) {
  if (t && /\S/.test(t)) {
    // Make CRLF and CR render one line break each.
    tipNode.setAttribute("label", t.replace(/\r\n?/g, "\n");
    return true;
  }
  return false;
);
[in which case you don't need retVal at all.]
Comment 12 Ian Neal 2012-05-20 15:31:49 PDT
Created attachment 625524 [details] [diff] [review]
moved defView patch

Changes since previous version:
* Moved defView to near beginning of function.
* Defined direction from defView.
* Checked against null in while loop.
Comment 13 Serge Gautherie (:sgautherie) 2012-05-20 20:25:03 PDT
Comment on attachment 625524 [details] [diff] [review]
moved defView patch

Review of attachment 625524 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for the tests.
Ftr, the 3 files (in this very patch) are identical to FF ones, except the url at end of browser_bug329212.js.

::: suite/browser/test/Makefile.in
@@ +57,5 @@
>  		test_registerHandler.html \
>  		$(NULL)
>  
> +_BROWSER_FILES = \
> +                 title_test.svg \

Nit: Sort title_test.svg to the end of the list.

::: suite/browser/test/browser/browser_bug329212.js
@@ +1,1 @@
> +function test () {

Nit: remove the extra space.

@@ +1,5 @@
> +function test () {
> +  waitForExplicitFinish();
> +  gBrowser.selectedTab = gBrowser.addTab();
> +  gBrowser.selectedBrowser.addEventListener("load", function () {
> +    gBrowser.selectedBrowser.removeEventListener("load", arguments.callee, true);

Nit: give the listener a name instead of using arguments.callee.

::: suite/browser/test/browser/browser_bug581947.js
@@ +50,5 @@
> +
> +  content.removeChild(e);
> +}
> +
> +function test () {

Nit: remove the extra space.

@@ +54,5 @@
> +function test () {
> +  waitForExplicitFinish();
> +  gBrowser.selectedTab = gBrowser.addTab();
> +  gBrowser.selectedBrowser.addEventListener("load", function () {
> +    gBrowser.selectedBrowser.removeEventListener("load", arguments.callee, true);

Nit: give the listener a name instead of using arguments.callee.

@@ +64,5 @@
> +      [ 'button',   true ],
> +      [ 'select',   false ],
> +      [ 'output',   true ],
> +      [ 'fieldset', true ],
> +      [ 'object',   true ],

Nit: remove the last ','.

@@ +72,5 @@
> +      check(data[0], data[1]);
> +    }
> +
> +    let todo_testData = [
> +      [ 'keygen', 'false' ],

Nit: remove the last ','.

@@ +75,5 @@
> +    let todo_testData = [
> +      [ 'keygen', 'false' ],
> +    ];
> +
> +    for each(let data in todo_testData) {

Nit: Add a space after 'each'.
Comment 14 Ian Neal 2012-05-21 15:41:21 PDT
Created attachment 625795 [details] [diff] [review]
named listener functions patch

Changes for tests carried out. Carrying forward r= for tests
Comment 15 Philip Chee 2012-05-23 11:42:16 PDT
Comment on attachment 625795 [details] [diff] [review]
named listener functions patch

> +      !tipElement.hasAttribute('title') &&
The rest of this function uses double quotes "title", etc.

> +    // If the element is barred from constraint validation or valid,
or is valid.

I found some tooltip test pages:

http://tests.petesguide.com/WebStandards/tests/tooltips.html
http://www.webdevout.net/testcases/title-tooltips/
Comment 16 Ian Neal 2012-06-03 09:56:14 PDT
Created attachment 629616 [details] [diff] [review]
Double instead of single quotes [Checked in: Comment 18]

Correct comment and change single to double quotes
Comment 17 neil@parkwaycc.co.uk 2012-06-14 08:25:34 PDT
Comment on attachment 629616 [details] [diff] [review]
Double instead of single quotes [Checked in: Comment 18]

Oops, sorry for overlooking this rerequest.

>+  let cought = false;
caught

>+  // 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") &&
>+      (!tipElement.form || !tipElement.form.noValidate)) {
>+    // If the element is barred from constraint validation or is valid,
>+    // the validation message will be the empty string.
>+    titleText = tipElement.validationMessage;
OK, so this breaks my null-checking idea somewhat. It's OK if the form element itself has the title, but if it doesn't, then the empty string validation message will stop the loop from finding a title on a parent element. I think we can fix this by suffixing an || null to convert the empty string to null.
Comment 18 Ian Neal 2012-06-17 14:09:04 PDT
Comment on attachment 629616 [details] [diff] [review]
Double instead of single quotes [Checked in: Comment 18]

Checked in with suggested null:
http://hg.mozilla.org/comm-central/rev/e5c33e9d0d7e

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