Closed Bug 623765 Opened 9 years ago Closed 9 years ago

Multiline links no longer render multiple highlights

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(fennec2.0+)

VERIFIED FIXED
Firefox 4.0
Tracking Status
fennec 2.0+ ---

People

(Reporter: stechz, Assigned: wesj)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

Click on any multiline link

Expected: multiple rectangles are used to highlight the link
Actual: only one rectangle is used
tracking-fennec: --- → 2.0+
Keywords: regression
Attached patch WIP (obsolete) — Splinter Review
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?
Attachment #502081 - Flags: review?(21)
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
Attached patch Patch v1Splinter Review
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.
Assignee: nobody → wjohnston
Attachment #502081 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #502581 - Flags: review?(21)
Attachment #502081 - Flags: review?(21)
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 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!
Attachment #502581 - Flags: review?(21) → review+
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.
Status: ASSIGNED → NEW
Pushed:

http://hg.mozilla.org/mobile-browser/rev/cdeef318a3a3
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
Depends on: 625832
Target Milestone: --- → Firefox 4.0
You need to log in before you can comment on or make changes to this bug.