623765 - Multiline links no longer render multiple highlights

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Benjamin Stover (:stechz)]

Click on any multiline link

Expected: multiple rectangles are used to highlight the link
Actual: only one rectangle is used

Comment 1

[Wesley Johnston (:wesj)]

Attachment: WIP

This is a WIP to move to using :active states for tap highlighting, rather than canvas, (adds mousedown because I built it on my patch in Bug 623763, not because I have some vendetta :) ).

Doesn't currently clear the highlight when you start panning. Curious if there was any feedback and we think this approach is worth pursuing?

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 502081 [details] [diff] [review]
WIP

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js
>   toString: function toString() {
>     return "[ContentTouchHandler] { }";
>-  }
>+  },
> };

Nit: no need for the extra ","


>diff --git a/chrome/content/content.css b/chrome/content/content.css
>+*:-moz-any-link:active,
>+*[role=button]:active,
>+button:active,
>+input:active,
>+option:active,
>+select:active,
>+textarea:active,
>+label:active {
>+  background-color: #8db8d8;
>+  outline: 1px dashed #8db8d8;
>+}

As said on IRC, I'm a bit afraid that some sites won't have any feedback, but after all it can't be a bad things if it was intentional.

>diff --git a/chrome/content/content.js b/chrome/content/content.js
> 
>+  _targetElt : null,
>+  get targetElt() {
>+    return this._targetElt;
>+  },
>+  set targetElt(val) {
>+    if (this._targetElt)
>+      gDOMUtils.setContentState(this._targetElt, 0);
>+    if (val)
>+      gDOMUtils.setContentState(val, 7);
>+    this._targetElt = val;
>+  },

Use constants instead of harcoded numbers, also I think you mentioned the :active state is not removed during a mouseup. I thought to have encounter this and my workaround was to give the :active state to the root of the document instead of trying to remove it.
If it works this could allow you to not have to stored the targetElement at all.

>       case "Browser:MouseOver": {
>         let element = elementFromPoint(x, y);
>         if (!element)
>           return;
> 
>         // Sending a mousemove force the dispatching of mouseover/mouseout
>         this._sendMouseEvent("mousemove", element, x, y);
>+        break;
>+      }
>+
>+      case "Browser:MouseDown": {
>+        let element = elementFromPoint(x, y);
>+        if (!element)
>+          return;

Please remove this :)

>         // There is no need to have a feedback for disabled element
>         let isDisabled = element instanceof HTMLOptionElement ? (element.disabled || element.parentNode.disabled) : element.disabled;
>         if (isDisabled)
>           return;
> 
>         // Calculate the rect of the active area
>-        let targetElement = null;
>+        this.targetElt = null;
>         if (element.mozMatchesSelector("*:-moz-any-link, *[role=button],button,input,option,select,textarea,label"))
>-          targetElement = element;
>+          this.targetElt = element;
>         else if (element.mozMatchesSelector("*:-moz-any-link *"))
>-          targetElement = element.parentNode;
>+          this.targetElt = element.parentNode;
>+        else
>+          this.targetElt = element;

I hope we don't need to store the target

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Thanks for the advice about setting this on the document. Works well.

This highlights active items by setting their background color, and well as adding a 2px blue outline. Hard to get a screenshot of, but I can set up something to fake it if Madhava wants/needs to see? Since we are just setting an attribute, I think we can be more lenient in what we allow to show.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 502581 [details] [diff] [review]
Patch v1

Drive by nits

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>         }
>         break;
> 
>-      case "Browser:Highlight": {
>-        let rects = aMessage.json.rects.map(function(r) new Rect(r.left, r.top, r.width, r.height));

Kill the blank line after the "break;" too

>diff --git a/chrome/content/content.js b/chrome/content/content.js

> Cu.import("resource://gre/modules/Services.jsm");
> 
>-let gFocusManager = Cc["@mozilla.org/focus-manager;1"]
>-  .getService(Ci.nsIFocusManager);
>+XPCOMUtils.defineLazyServiceGetter(this, "gFocusManager",
>+  "@mozilla.org/focus-manager;1", "nsIFocusManager");
>+XPCOMUtils.defineLazyServiceGetter(this, "gDOMUtils",
>+  "@mozilla.org/inspector/dom-utils;1", "inIDOMUtils");

Cu.import XPCOMUtils.jsm too

>-        else if (element.mozMatchesSelector("*:-moz-any-link *"))
>-          targetElement = element.parentNode;
>+        this._doTapHighlight(element);
> 
>-        if (targetElement) {

Kill the blank line after _doTapHighlight

>+  _doTapHighlight: function _doTapHighlight(aElt) {
>+    gDOMUtils.setContentState(aElt, EVENT_STATES_ACTIVE);
>+  },

aElt -> aElement

Nice cleanup!

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 502581 [details] [diff] [review]
Patch v1

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

> 
>   /** Invalidates any messages received from content that are sensitive to time. */
>   _clearPendingMessages: function _clearPendingMessages() {
>     this._messageId++;
>-    TapHighlightHelper.hide(0);
>-    this._contextMenu = null;

Do we need to remove the this._contextMenu property?

> 
>   tapUp: function tapUp(aX, aY) {
>-    TapHighlightHelper.hide(200);
>-    this._contextMenu = null;

Same question here

>+    let browser = getBrowser();
>+    browser.messageManager.sendAsyncMessage("Browser:MouseCancel", {});
>   },
> 
>   tapSingle: function tapSingle(aX, aY, aModifiers) {
>-    TapHighlightHelper.hide(200);
>-    this._contextMenu = null;
>-

Same here

>diff --git a/chrome/content/content.js b/chrome/content/content.js

> 
>+const EVENT_STATES_ACTIVE = 1;

For consistency I would like to use the same name in both places, I don't have a preference for the most valid.
http://mxr.mozilla.org/mobile-browser/source/chrome/content/input.js#60


r+ with nits adressed (and Mark's comments too)

I like the idea of using the :active state of the element!

Comment 6

[Wesley Johnston (:wesj)]

I thought (and a quick mxr search made me believe) that this._contextMenu was unused now. Are you aware of anything around that uses it.

Comment 7

[Wesley Johnston (:wesj)]

Pushed:

http://hg.mozilla.org/mobile-browser/rev/cdeef318a3a3

Comment 8

[ahoza]

Verified as fixed on:
Mozilla /5.0 (Maemo;Linux armv7l;rv:2.0b9pre)Gecko/20110112 Firefox/4.0b9pre Fennec /4.0b4pre
and 
Mozilla /5.0 (Android;Linux armv7l;rv:2.0b9pre) Gecko/20110112 Firefox/4.0b9pre Fennec /4.0b4pre