Closed Bug 601091 Opened 14 years ago Closed 12 years ago

Port FillInHTMLTooltip changes from Firefox (e.g. HTML5 form validation)

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.13

People

(Reporter: bugzilla, Assigned: iannbugzilla)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Attachment #480070 - Flags: feedback?
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;
> }
Attachment #480070 - Flags: feedback? → feedback?(iann_bugzilla)
q.v. Bug 605277 - Do not show the error message in the tooltip if title is set
Depends on: 605277
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.
>>+      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."
> +      tipNode.setAttribute("label", t);
Hmm. I thought that if we want the text to wrap we should be using .textContent?
q.v. Toolkit Bug 655747 - Add BrowserUtils.jsm for commonly used browser-related code
Depends on: 655747
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?
Summary: Port FillInHTMLTooltip changes from firefox → Port FillInHTMLTooltip changes from Firefox (e.g. HTML5 form validation)
>>>+      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
See Also: → 358452, 721142
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.
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.
Assignee: aqualon → iann_bugzilla
Attachment #480070 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #625484 - Flags: review?(sgautherie.bz)
Attachment #625484 - Flags: review?(neil)
Attachment #625484 - Flags: feedback?(sgautherie.bz)
Attachment #625484 - Flags: feedback?(philip.chee)
Attachment #480070 - Flags: feedback?(iann_bugzilla)
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.]
Attached patch moved defView patch (obsolete) — Splinter Review
Changes since previous version:
* Moved defView to near beginning of function.
* Defined direction from defView.
* Checked against null in while loop.
Attachment #625484 - Attachment is obsolete: true
Attachment #625524 - Flags: review?(sgautherie.bz)
Attachment #625524 - Flags: review?(neil)
Attachment #625524 - Flags: feedback?(philip.chee)
Attachment #625484 - Flags: review?(sgautherie.bz)
Attachment #625484 - Flags: review?(neil)
Attachment #625484 - Flags: feedback?(sgautherie.bz)
Attachment #625484 - Flags: feedback?(philip.chee)
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'.
Attachment #625524 - Flags: review?(sgautherie.bz) → review+
Component: General → UI Design
Depends on: 358452, 581947, 329212, 595922, 606491, 639945, 693551, 715999, 745712, 738233
No longer depends on: 599745, 605277, 655747
QA Contact: general → ui-design
See Also: 358452
Target Milestone: --- → seamonkey2.12
Attached patch named listener functions patch (obsolete) — Splinter Review
Changes for tests carried out. Carrying forward r= for tests
Attachment #625524 - Attachment is obsolete: true
Attachment #625524 - Flags: review?(neil)
Attachment #625524 - Flags: feedback?(philip.chee)
Attachment #625795 - Flags: review?(neil)
Attachment #625795 - Flags: feedback?(philip.chee)
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/
Attachment #625795 - Flags: feedback?(philip.chee) → feedback+
Correct comment and change single to double quotes
Attachment #625795 - Attachment is obsolete: true
Attachment #625795 - Flags: review?(neil)
Attachment #629616 - Flags: review?(neil)
Attachment #629616 - Flags: feedback+
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.
Attachment #629616 - Flags: review?(neil) → review+
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
Attachment #629616 - Attachment description: Double instead of single quotes → Double instead of single quotes [Checked in: Comment 18]
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: seamonkey2.12 → seamonkey2.13
Depends on: 779720
No longer depends on: 779720
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: