Status

defect
RESOLVED WORKSFORME
10 years ago
9 years ago

People

(Reporter: vingtetun, Assigned: vingtetun)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(fennec1.0-)

Details

Attachments

(3 attachments, 5 obsolete attachments)

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
No longer blocks: 492546
Summary: Buttons doesn't show a press state → Buttons doesn't look pressed
Assignee: nobody → combee
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?
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.
(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.
Posted patch Update patch (obsolete) — Splinter Review
This patch remove the blur workaround and respect the default css behavior for buttons.
Attachment #378177 - Attachment is obsolete: true
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.
Posted patch Updated patch (obsolete) — Splinter Review
Call endPressingState in cancelPending
Attachment #378351 - Attachment is obsolete: true
Blocks: 477628
No longer blocks: 492546
tracking-fennec: --- → ?
(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
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?
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.
Posted patch Handle radio buttons (obsolete) — Splinter Review
Handle radio buttons
Attachment #378564 - Attachment is obsolete: true
Status: UNCONFIRMED → NEW
Ever confirmed: true
Viven - If you can reduce the amount of code in the patch by depending on bug 487693, please do it.
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...
Attachment #382099 - Attachment is obsolete: true
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?
Assignee: combee → 21
(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?
wrong 'depends on'
Depends on: 458741
No longer depends on: 460966
Posted patch Updated patchSplinter Review
(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
Attachment #382271 - Attachment is obsolete: true

Updated

10 years ago
Summary: Buttons doesn't look pressed → Buttons don't look pressed
Dispatch hover to all elements and if something in the display have changed (css dropdown menu) prevent clicking
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?
[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.

Updated

10 years ago
tracking-fennec: ? → 1.0-
Is this working now? I see the button is changed after it's clicked on.
There is a blue highlight on buttons now so we don't need that.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.