Closed Bug 921824 Opened 11 years ago Closed 11 years ago

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

Categories

(Firefox OS Graveyard :: Gaia, defect)

Product:
Firefox OS Graveyard
Component:
Gaia
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: vingtetun, Assigned: vingtetun)

References

Details

Attachments

(1 file, 3 obsolete files)

use_hover.patch
3.54 KB, patch
arnau
: review+
Details | Diff | Splinter Review
Attached patch highlight.poc.patch (obsolete) — — Splinter Review 
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).
Attachment #811660 - Flags: feedback?(anthony)
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.
(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...
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.
-----
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.
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.
(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...
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.
Attached patch always.display.active.patch (obsolete) — — Splinter Review 
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.
Attached patch hover.patch (obsolete) — — Splinter Review 
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.)
Attachment #811660 - Attachment is obsolete: true
Attachment #811660 - Flags: feedback?(anthony)
Attachment #811679 - Flags: feedback?(anthony)
A long time ago, I opened bug 863702 to get Gecko to fix :active.
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.
I've found a situation where using :hover could be annoying: inside Firefox desktop.
(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?
(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.
(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.
Attachment #811679 - Flags: feedback?(anthony)
Sam do you have some free time to use the :hover stuff and land that?
Flags: needinfo?(sjochimek)
Attached patch use_hover.patch — — Splinter Review 
Arnau let me know if this is enough of if you want to add more changes...
Attachment #811675 - Attachment is obsolete: true
Attachment #811679 - Attachment is obsolete: true
Attachment #816680 - Flags: review?(arnau)
I removed the transition because with :hover we don't need it.
Comment on attachment 816680 [details] [diff] [review]
use_hover.patch

Looks good to me :)
Attachment #816680 - Flags: review?(arnau) → review+
(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.
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?
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Flags: needinfo?(sjochimek)
Assignee: nobody → 21
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: