841228 - Defect - Hover state on buttons (like the bookmark star) persists after you tap the button

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Input, defect, P1)

Description

[juan becerra [:juanb]]

Tested on 2013-02-13 Nightly built from http://hg.mozilla.org/project/elm/rev/b3e65fe37681

If you bookmark a page, and then unbookmark it, then the unbookmarked star is displayed in a sort of in-between state (although the actual state is unbookmarked).

This goes away if you open the site in question in another tab. When you bring up the context app tab it shows the correct display for the star button.

Comment 1

[Matt Brubeck (:mbrubeck)]

This is actually the hover state for the button.  Tapping on the button simulates a mouse click on the button, and after you finish tapping, the "mouse" cursor is still hovering over the button, although you can't see it because we hide the cursor during touch input.

This will affect any element with a hover state.

Comment 2

[Jim Mathies [:jimm]]

This shows up on all of our context menus as well.

Comment 3

[Asa Dotzler [:asa]]

We gotta solve this problem. It's really ugly. Do I need to create a story saying "don't do this" or is there some existing place to hook up this bug?

Comment 4

[Brian R. Bondy [:bbondy]]

I think we should make this an it5 defect blocking story bug 756438.
Bug 756438 would have been a story if we would have had iterations back then.

Comment 5

[Brian R. Bondy [:bbondy]]

Given comment 3 I'll mark it as a defect of it5 but please re-adjust if you disagree.

Comment 6

[Marco Mucci [:MarcoM]]

Yes, we'll put this forward as a defect for iteration #5.

Comment 7

[Brian R. Bondy [:bbondy]]

Attachment: Path v1

There's a couple of other possible fixes I got to work in content\events\src\nsEventStateManager.cpp but I prefer to do the fix here because in Desktop programs (not just Firefox) a tap implies a mouse cursor move, but the same is not true in Metro programs (not just Metro Firefox).

Comment 8

[Brian R. Bondy [:bbondy]]

Also this seems to be the safest thing to do to avoid breaking web content.

Comment 9

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

See also bug 856222 for a related (identical?) issue, but specific to stylus/pen input.

Comment 10

[Jim Mathies [:jimm]]

http://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroInput.cpp#1245

Can we reuse the refPoint we already have in mouseEvent? I think there is a coordinate space issue here - GetCursorPos Doesn't provide physical coordinates, and I'm not sure what ScreenToClient handles.

Comment 11

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2.

Updated as per discussion.
- Check result of GetCursorPos assuming that it will fail if there's no mouse
- Now use physical coords and then conver to logical afterwards

Tested with the make things bigger option at 125% and at 100%
Tested snapping Firefox left and right

What I tested in each DPI was having the hover state on a link so it shows an underline then tapping on an appbar button. After the tap the underline was preserved.  Then tested moving the cursor on top of the button I was tapping and the hover state was preserved on that appbar button.

Comment 12

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ababa309ae8b

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ababa309ae8b

Comment 14

[raymond [:retornam] (inactive)]

verified fixed  April 8th, 2012  built from http://hg.mozilla.org/mozilla-central/rev/b0d842380959

Comment 15

[raymond [:retornam] (inactive)]

Bookmarked and un-bookmarked a couple of pages in metro mode and  could no longer reproduce this

Comment 16

[Kamil Jozwiak [:kjozwiak]]

Went through the following "Defect" for iteration #5 testing and everything passed without any issue. Used the following build:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-04-14-03-10-25-mozilla-central/

- Bookmarked several pages by tapping the bookmark "Star" and ensured that the "hover" state is cleared
- Tapped on the "Download" button several times and ensured that the "hover" state is not being used
- Tapped in the "Pin" several times and ensured that the "hover" state is not being used
- Tapped the "Previous" and "Next" buttons when searching for text and ensured that the "hover" state is not being used
- Tested all the controls mentioned above in filled view
- Ensured that the "hover" state is activated/deactivated with the mouse pointer
- Tested all the controls mentioned above worked correctly with both touch/mouse with "Always Show Tabs" in the "Settings"

Don't have a stylus/pen so couldn't test the above cases using them.

Comment 17

[Kamil Jozwiak [:kjozwiak]]

Went through the following "Defect" for iteration #6 testing. Used the following build:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-04-30-03-09-41-mozilla-central/

Went through all of the previous comments and ensured everything worked without any issues. (including Comment 16)

Don't have a stylus/pen so couldn't test those test cases mentioned Comment 9

Comment 18

[Samvedana (:Samvedana)]

Attachment: app_bar state.png

User Agent: Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130812030209
Built from http://hg.mozilla.org/mozilla-central/rev/87c1796bc46c


Tested on windows 8 using latest nightly for iteration-11. Followed steps provided in comment16. I see the attached state.I don't know exactly if it is desired behavior.

Comment 19

[Brian R. Bondy [:bbondy]]

I think that's a bug, but please check with Yuan. 
If it is a bug, then please post a new bugzilla bug for it.

Comment 20

[Samvedana (:Samvedana)]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130825030201
Built from http://hg.mozilla.org/mozilla-central/rev/01576441bdc6

WFM
Tested on windows 8 using latest nightly for iteration-12. Followed steps provided in comment0 and got expected result.