Closed Bug 510857 Opened 11 years ago Closed 10 years ago

Buttons don't look pressed in bookmarks, preferences, downloads, ...

Categories

(Firefox for Android Graveyard :: Bookmarks, defect)

defect
Not set

Tracking

(fennec1.0+)

VERIFIED FIXED
Tracking Status
fennec 1.0+ ---

People

(Reporter: vingtetun, Assigned: vingtetun)

References

Details

Attachments

(1 file, 5 obsolete files)

Attached patch Patch v0.1 (obsolete) — Splinter Review
Since the tile merge the buttons in bookmarks, preferences, ... don't look pressed.

This is cause of the way we define allowRealtimeDownUp.

This patch rename allowRealtimeDownUp to propagateDownUp and instead of using a property on the dragger js object, used an attribute on the scrollable element ('propagatedownup').
Attachment #394789 - Flags: review?(mark.finkle)
Attachment #394789 - Flags: review?(froystig)
Comment on attachment 394789 [details] [diff] [review]
Patch v0.1

You removed from the big comment above MouseModule in InputHandler.js the documentation on the property.  Since this comment is currently the only documentation on MouseModule's API to scrollables, would be nice to have a new one in its place to document the attribute you created --- just a sentence like the old one is fine.  r+ with that.
Attachment #394789 - Flags: review?(froystig) → review+
Attached patch Patch v0.2 (obsolete) — Splinter Review
Roy, Thanks for review.
After a discussion with Mark we have decided to change a bit the implementation, is that good for you? Is the comment good enough (it can be too frenchy :) ) ?
Attachment #394789 - Attachment is obsolete: true
Attachment #394816 - Flags: review?(mark.finkle)
Attachment #394816 - Flags: review?(froystig)
Attachment #394789 - Flags: review?(mark.finkle)
Comment on attachment 394816 [details] [diff] [review]
Patch v0.2

>+
>+  preventDownUp: function propagateDownUp() {

Looks like a typo on the actual function name (as opposed to the prototype property name).  I wouldn't have mentioned it if the two names didn't happen to be in contradiction.  :)

r+ with that.
Attachment #394816 - Flags: review?(froystig) → review+
Attached patch Patch V0.3 (obsolete) — Splinter Review
(In reply to comment #3)
> (From update of attachment 394816 [details] [diff] [review])
> >+
> >+  preventDownUp: function propagateDownUp() {
> 
> Looks like a typo on the actual function name (as opposed to the prototype
> property name).  I wouldn't have mentioned it if the two names didn't happen to
> be in contradiction.  :)

Corrected!
Thanks for the review.
Attachment #394931 - Flags: review?(mark.finkle)
Comment on attachment 394931 [details] [diff] [review]
Patch V0.3

My only nit is: preventDownUp should be a property, not a function, since it's a state flag and not a function that actually prevent a down or up.
Attachment #394931 - Flags: review?(mark.finkle) → review+
Attached patch Patch v0.4 (obsolete) — Splinter Review
* Change preventDownUp from method to property
Attachment #395024 - Flags: review?(mark.finkle)
Attachment #395024 - Flags: review?(mark.finkle) → review+
Attachment #394816 - Attachment is obsolete: true
Attachment #394931 - Attachment is obsolete: true
tracking-fennec: --- → ?
Attached patch Patch v0.5 (obsolete) — Splinter Review
After have played with my patch a lot, i've found that it's rotted.
I mean, the preventDownUp is based on the ownerDocument of the event target, but each event came from chrome...
So, the patch never prevent down/up to be dispatched.
In fact that's not very important since the down/up are dispathed to the html:canvas of the tile-container not to the content itself.

Roy, can you confirm this has no importance?

Secondly, and I suspect that the true reason why we prevent dispatching sometimes, panning the awesome list, or anything that implement a custom dragger, fire a click if during the panning the element under the mouse hasn't changed. I suppose that's the problem encounterd by Mark.

What i propose, is to remove all the preventDefault/stopPropagation on mousedown/mouseup (if (this._dragger....) and let the event propagate, and to bypass the 'click' problem for dragger, we can check the evInfo.event.detail of the mouseup to know if we're going to generate a click, and in this case call suppressNextClick.
Attachment #395024 - Attachment is obsolete: true
Attachment #396418 - Flags: review?(combee)
Attachment #396418 - Flags: review?(froystig)
To answer your question:  Yes, preventing up/down to content is just preventing an up and down on the tiles in the tile-container, not the content itself.

Proposal sounds fine.  The only downside I can see to it (and I may be wrong) is that by allowing the propagations, then e.g. in panning the awesomebar list, something may appear visually "depressed" while it is being dragged, since it got a mousedown.  If that happens and is considered okay, then with this patch it at least won't get clicked, which seems like good times to me.  I haven't checked that claim.

The event.detail of a MouseEvent being a click counter is news to me --- that's quite useful.  MDC documents the event.detail field as the number of "times the mouse has been clicked in the same location for this event", which sounds correct to me.  (https://developer.mozilla.org/en/DOM/event.detail)

Review on the patch coming right up.
Comment on attachment 396418 [details] [diff] [review]
Patch v0.5

>   /**
>+   * Check if the event concern the browser content
>+   */
>+  _targetIsContent: function _isEventForContent(aEvent) {
>+    let tileBox = document.getElementById("tile-container");
>+    let target = aEvent.target;
>+    while (target) {
>+      if (target === window)
>+        return false;
>+      if (target === tileBox)
>+        return true;
>+
>+      target = target.parentNode;
>+    }
>+    return false;
>+  },
>+

Regarding |document.getElementById("tile-container")|, I'd rather avoid having surprising dependencies from InputHandler relying on a hard-coded name of the tile container element as it is in browser.xul.  Only browser.js should be coupled tightly enough with browser.xul to be allowed to refer to elements by name.  Here, I'd rather the MouseModule be given a constructor argument |browserViewContainer| (or some other name) that is the tile-container element object, cached this object, and used it in this function.  If you want, you can use the |browserViewContainer| variable that's already passed to the InputHandler constructor.

Otherwise great!  r+ with the change.
Attachment #396418 - Flags: review?(froystig) → review+
(In reply to comment #8)
> To answer your question:  Yes, preventing up/down to content is just preventing
> an up and down on the tiles in the tile-container, not the content itself.
> 
> Proposal sounds fine.  The only downside I can see to it (and I may be wrong)
> is that by allowing the propagations, then e.g. in panning the awesomebar list,
> something may appear visually "depressed" while it is being dragged, since it
> got a mousedown.

Yep, that's not very "sexy".

  If that happens and is considered okay, then with this patch
> it at least won't get clicked, which seems like good times to me.  I haven't
> checked that claim.
> 
> The event.detail of a MouseEvent being a click counter is news to me --- that's
> quite useful.  MDC documents the event.detail field as the number of "times the
> mouse has been clicked in the same location for this event", which sounds
> correct to me.  (https://developer.mozilla.org/en/DOM/event.detail)
> 

I've to admit that I've not read MDC but that: http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventStateManager.cpp#3782

> Review on the patch coming right up.
(In reply to comment #9)
> (From update of attachment 396418 [details] [diff] [review])
> >   /**
> >+   * Check if the event concern the browser content
> >+   */
> >+  _targetIsContent: function _isEventForContent(aEvent) {
> >+    let tileBox = document.getElementById("tile-container");
> >+    let target = aEvent.target;
> >+    while (target) {
> >+      if (target === window)
> >+        return false;
> >+      if (target === tileBox)
> >+        return true;
> >+
> >+      target = target.parentNode;
> >+    }
> >+    return false;
> >+  },
> >+
> 
> Regarding |document.getElementById("tile-container")|, I'd rather avoid having
> surprising dependencies from InputHandler relying on a hard-coded name of the
> tile container element as it is in browser.xul.  Only browser.js should be
> coupled tightly enough with browser.xul to be allowed to refer to elements by
> name.  Here, I'd rather the MouseModule be given a constructor argument
> |browserViewContainer| (or some other name) that is the tile-container element
> object, cached this object, and used it in this function.  If you want, you can
> use the |browserViewContainer| variable that's already passed to the
> InputHandler constructor.
> 
> Otherwise great!  r+ with the change.

That's cleaner. I'll do the change once Ben has finished his review.

Thanks for you review!
Attachment #396418 - Flags: review?(combee) → review+
tracking-fennec: ? → 1.0+
Attachment #396742 - Flags: review? → review+
Attachment #396742 - Flags: review+ → review?(froystig)
Attachment #396742 - Flags: review?(froystig) → review+
Blocks: 477628
I think we all want this bug fixed and the best way to see if the behavior in this patch is good enough is to land it. It has enough reviews, so we can file bugs on the behavior if we don't like it. Worse case, we can back it out.
pushed:
https://hg.mozilla.org/mobile-browser/rev/17e003d51153
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Verified on n810 with 1.9.2 nightly build 20090921
Status: RESOLVED → VERIFIED
Component: General → Bookmarks
You need to log in before you can comment on or make changes to this bug.