492831 - Buttons don't look pressed

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: General, defect)

Description

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

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090511 Minefield/3.6a1pre
Build Identifier: 

When i click on a button and keep the button/finger 'pressed', buttons doesn't change their graphic state

Reproducible: Always

Steps to Reproduce:
1.Go to google.Fr
2.Key pressing the search button
Actual Results:  
Button doesn't look pressed

Expected Results:  
Button should look pressed

Comment 1

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

Attachment: Allow button to show they pressed state on web content

Patch to allow input[type=submit|reset|button] to show a pressed state in web content.
There is some workaround like a force blur event, cause a click on a previously focused button doesn't seems to fire any MozAfterPaint event.
I've add a module but I'm not sure if it should be better to do it inside ContentClickingModule instead?

Comment 2

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

CC'ing Ben to look at the possibility of adding to the content handler.

Although, we should really look into why we don't get a MozAfterPaint event.

Comment 3

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

(In reply to comment #2)

> Although, we should really look into why we don't get a MozAfterPaint event.

It seems that MozAfterPaint is not throw cause of the css declaration in res/forms.css#515.  There is no css declaration for only button:active but instead it's button:active:hover, so if I change this all works (but it's not the right way).
I'm gonna improve my patch for take that in account.

Comment 4

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

Attachment: Update patch

This patch remove the blur workaround and respect the default css behavior for buttons.

Comment 5

[Ben Combee (:bcombee)]

The patch new inputhandler doesn't call Browser.canvasBrowser.stopPressingState() in cancelPending... this means that it would get confused if you started a drag while over an <input> element.

Comment 6

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

Attachment: Updated patch

Call endPressingState in cancelPending

Comment 7

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

(In reply to comment #3)
> (In reply to comment #2)
> 
> > Although, we should really look into why we don't get a MozAfterPaint event.
> 
> It seems that MozAfterPaint is not throw cause of the css declaration in
> res/forms.css#515.  There is no css declaration for only button:active but
> instead it's button:active:hover, so if I change this all works (but it's not
> the right way).
> I'm gonna improve my patch for take that in account.

Tell me more about why you think the CSS at res/forms.css#515 is affecting MozAfterPaint

Comment 8

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

Oops I had another question:

(In reply to comment #3)
> (In reply to comment #2)
> 
> > Although, we should really look into why we don't get a MozAfterPaint event.
> 
> It seems that MozAfterPaint is not throw cause of the css declaration in
> res/forms.css#515.  There is no css declaration for only button:active but
> instead it's button:active:hover, so if I change this all works (but it's not
> the right way).

What did you change? The CSS? What change did you make?

Comment 9

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

Sorry for my lack of explanations :(

(In reply to comment #8)

> Tell me more about why you think the CSS at res/forms.css#515 is affecting
MozAfterPaint

I'm not trying to say that the CSS declaration affect the MozAfterPaint event himself but when a mouseup event is dispatched to a button, the button's style is not changed, except if it's already in a hover state (button:active:hover). 

And so, cause no style is changed, there is no need to send a MozAfterPaint event (I've dump all MozAfterPaint coming in nsWindow (::OnPaint event on my linux box)  and there is none.


> What did you change? The CSS? What change did you make?

I didn't change anything in the CSS, I've just dispatched a mouseover before the mouseup to force the button to enter in an :hover state.
And to have a consistent behavior with Firefox (not sure this is needed?), I'm listening the mousemove and dispatching them (and preventing panning) until the user released his mouse/finger. This lead to a button that look pressed when the mouse/finger is over and unpressed when not.

Comment 10

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

Attachment: Handle radio buttons

Handle radio buttons

Comment 11

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

Viven - If you can reduce the amount of code in the patch by depending on bug 487693, please do it.

Comment 12

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

Attachment: Updated patch depending on bug 487693

Removed the changed done in CanvasBrowser.js (depending on patch 487693)
Add a few lines for handling XUL [button|checkbox|radio] rendered directly in canvas

Not sure of this but I've had the handling of label element, actually it seems that when nsHTMLLabelElement.cpp#295 is fired we are not in a allowNextClick...

Comment 13

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

This patch is looking pretty good. Some comments:
* ContentDownUpModule ->ContentHoverModule (since we are kind of simulating :hover)
* Ben has a patch that should make elementFromPoint work from iframes
* Bug 460966 is about mouseover affect DHTML menus. This patch might fix that too, if you allowed more HTML types. What's the downside to allowing any HTML element? Can you test this?

Comment 14

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

(In reply to comment #13)
> This patch is looking pretty good. Some comments:
> * ContentDownUpModule ->ContentHoverModule (since we are kind of simulating
> :hover)

You're right, I'll change that.

> * Ben has a patch that should make elementFromPoint work from iframes

Ok, depends on 458541 for that.

> * Bug 460966 is about mouseover affect DHTML menus. This patch might fix that
> too, if you allowed more HTML types. What's the downside to allowing any HTML
> element? Can you test this?

At first sight I see:
 * If I allow all HTML elements to be handle through this module, this lead to steal all click from the actual ContentClickingModule :(
 * After testing a bit, activating all HTML element turn the menu from bug 460966 to open on mousedown, but :
  * If we directly release the mouse/finger we do a click on the top menu - that can be what we want?
  * If we move to an item in the submenu and release the mouse/finger we just throw a mouseup and not a click cause of the distance between the mousedown and the mouseup.

I suppose what we want for bug 460966 is something like the iphone (as described in it) :
 * When we 'click' on something which open a menu we want to inhibit the click - it certainly be easier for the user to have the ability to click and open the menu instead of holding down?
 * Let it dblclick to open the menu but this collapse with the zoom mechanism

I can certainly hack on top of this patch to make something that works for bug 460966 but I'm not sure of the wanted behavior?

Comment 15

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

wrong 'depends on'

Comment 16

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

Attachment: Updated patch

(In reply to comment #13)
> This patch is looking pretty good. Some comments:
> * ContentDownUpModule ->ContentHoverModule (since we are kind of simulating
> :hover)

Done.

> * Ben has a patch that should make elementFromPoint work from iframes

Ok. I've test this patch with Ben's patch for elementFromPoint and now it works with input nested in iframes

Comment 17

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

Attachment: draft for dispatch hover to all elements

Dispatch hover to all elements and if something in the display have changed (css dropdown menu) prevent clicking

Comment 18

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

Attachment: Image of the events flow with the patch

It's basically how the events of InputHandler works (as I've understand) and how my last patch for this bug and bug 460966 affect the working flow. 
(I've not take into account dblclick here and any stuff from ChromeInputModule.)

Mark, Ben what do you think of that? Did I've done some mistakes?

Comment 19

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

[17:01]	<bcombee>	on the mousemove... I think we should emulate the iphone behavior -- they synthesize mousemoves to go with clicks
[17:02]	<bcombee>	I'd rather not have the desktop act differently
[17:02]	<bcombee>	so each click comes with a mousemove proceeding it
[17:03]	<mfinkle>	vingtetun: that is similar to your "button press" fix, right
[17:03]	<mfinkle>	except you only send the mousemove when clicking on certain elements
[17:04]	<bcombee>	the iphone sends mousemove everywhere
[17:04]	<bcombee>	from what I've read


With the previous patch I dispatch a mouseover event to all elements without exception (ok... except for nsIDOMHTMLLabelElement but I can change that) before a mousedown (even if the mousedown is not followed by a mouseup (click)).

As suggested we can change it by a mousemove and this probably lead to a mouseover that fired.

The real tricky part is to know when we should prevent the click to fire. Actually I search for display:none diff on ul elements, and as Mark have say it's far from a solid solution.
I'm very open to all suggestions here.

Comment 21

[Aakash Desai [:aakashd]]

Is this working now? I see the button is changed after it's clicked on.

Comment 22

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

There is a blue highlight on buttons now so we don't need that.