921824 - Highlight color is often not visible after a click on an element

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

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

Attachment: highlight.poc.patch

It happens all hover Gaia and defeat the purpose of highlighting where |highlight == action| and |action == highlight|. 

The main reason why it happens is because the :active class is triggered after a timeout after a touchstart, but if the user release the finger before this timeout then the class is never applied.

Tweaking the building blocks to also show the highlighting color on a :hover solve most of the issue since in this case, even if the user has click faster than the timeout, the sequence of |mousedown,mousemove,mouseup| that will be triggered to result into a click will activate this rule.

Asking feedback to Anthony since he spends some time looking at that and may help me to turn the POC into a real patch (hint).

Comment 1

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

fwiw since the highlight will be done before any |click| handler, it is likely that it becomes parts of any texture that is constructed for transitions. As a result it will looks like the highlight is staying here for the duration of the transition between 2 panels. Which is even better imho.

Comment 2

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

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #1)
> fwiw since the highlight will be done before any |click| handler, it is
> likely that it becomes parts of any texture that is constructed for
> transitions. As a result it will looks like the highlight is staying here
> for the duration of the transition between 2 panels. Which is even better
> imho.

It could also be because the :hover class persist after a click. That sounds like a bug in BrowserElementPanning.js. Should be fixable but right now my Gecko build is broken so I can not play with it...

Comment 3

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

This is likely because the platform think the mouse is still at the same place after a click and the element never receive a mouseout/mouseleave event. I wonder what the spec says about that.

Comment 4

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

-----
7. Interaction with Mouse Events
 
...

If the user agent intreprets a sequence of touch events as a click, then it should dispatch mousemove, mousedown, mouseup, and click events (in that order) at the location of the touchend event for the corresponding touch input. If the contents of the document have changed during processing of the touch events, then the user agent may dispatch the mouse events to a different target than the touch events.

The default actions and ordering of any further touch and mouse events are implementation-defined, except as specified elsewhere. 

-----

The last paragraph seems what we are looking for since the spec does not speak of any mousenter/mouseout/mouseleave events.

Comment 5

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

Maybe we don't even need to fire and event since the finger is not *anymore* on the screen after a touchend it seems odd to keep the |:hover| class at all. I assume this one can be remove safely on/after a touchend.

Comment 6

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

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #5)
> Maybe we don't even need to fire and event since the finger is not *anymore*
> on the screen after a touchend it seems odd to keep the |:hover| class at
> all. I assume this one can be remove safely on/after a touchend.

Sigh. I assume that keeping :hover on the element on a click is for :hover drop down menu and to keep them opened after a click. hm...

Comment 7

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

OK. I think I see a pattern here:
 1. if the element will be moved out of view by the click. Then let's use :hover.
 2. if the element will stay on the screen then let's use :active.

Then the result will be: 
 R1. elements that will be move out of view will be highlighted before and during the transition. 
 r2. elements that will stay will be highlighted quickly before returning to normal state.

To achieve 1/R1 the change needs to happen on the building blocks side. Adding the :hover pseudo selector where it makes sense and use it wisely should do it.
A small platform fix to remove the :hover class on element when the application visibility is turned off may follow.

2/R2 happens more or less (and often less). The cause of that is related to the timeout mechanism to trigger the :active class that does not happen if the user click quickly. Fixing BrowserElementPanning.js to force the :active class on the touchend if the timeout has not been reached and to remove it right after should fix that in theory.

Comment 8

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

Attachment: always.display.active.patch

Something like this for Gecko makes me sad but I don't have any better idea. This will guarantee that there any nodes will always have a small delay to be :active.

Comment 9

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

Attachment: hover.patch

This is the Gaia patch I'm playing with. 

With both patches applied the UX seems much better too me.  Also with the Gecko patch it is obvious to see where application logic is responsible for highlight failures (contacts, browser, gallery, some select in settings, etc.)

Comment 10

[Anthony Ricaud (:rik)]

A long time ago, I opened bug 863702 to get Gecko to fix :active.

Comment 11

[Anthony Ricaud (:rik)]

For 1, we decided to use :focus in the Settings app. I can't remember why we decided not to use :hover. Especially because with :focus, we can't use hashes for app states (the element with the id=hash would get the focus instead of the element with href=hash).

For 2, I think Fx Android would also benefit. So fixing bug 863702 seems more valuable to me.

Comment 12

[Anthony Ricaud (:rik)]

I've found a situation where using :hover could be annoying: inside Firefox desktop.

Comment 13

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

(In reply to Anthony Ricaud (:rik) from comment #12)
> I've found a situation where using :hover could be annoying: inside Firefox
> desktop.

It should not be if the FF desktop work with touch events. Maybe wrapping those into a media query that check touch event supports is enough?

Comment 14

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

(In reply to Anthony Ricaud (:rik) from comment #11)
> For 1, we decided to use :focus in the Settings app. I can't remember why we
> decided not to use :hover. Especially because with :focus, we can't use
> hashes for app states (the element with the id=hash would get the focus
> instead of the element with href=hash).
> 

Does this comment means you think :focus is better in your opinion? Preventing the application to use the hash sounds bad enough since this is a common web practice though.

Comment 15

[Anthony Ricaud (:rik)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #14)
> Does this comment means you think :focus is better in your opinion?
> Preventing the application to use the hash sounds bad enough since this is a
> common web practice though.
No no, I think :hover is better. I was just voicing the limitations of :focus.

I'd love to use :hover all over the place but we need to find a nice solution for Fx desktop. Media queries are not nice because that means duplicating the selectors and makes it hard to maintain the CSS.

Comment 16

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

Sam do you have some free time to use the :hover stuff and land that?

Comment 17

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

Attachment: use_hover.patch

Arnau let me know if this is enough of if you want to add more changes...

Comment 18

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

I removed the transition because with :hover we don't need it.

Comment 19

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Comment on attachment 816680 [details] [diff] [review]
use_hover.patch

Looks good to me :)

Comment 20

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

(In reply to Arnau March from comment #19)
> Comment on attachment 816680 [details] [diff] [review]
> use_hover.patch
> 
> Looks good to me :)

Cool. I will land in a few since the platform patch has just hit mozilla-central.

Comment 21

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

https://github.com/mozilla-b2g/gaia/commit/ab67baafae21235e7d2a7ecf756d7d54232b41c1

Sam there are many places inside apps where there is no highlighting (application bugs). Can you have a look at them?