950623 - [Messages] Multi-recipient. Back button is kept as pressed after going back from recipients details list

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[isabelrios]

v1.3 12/16 build:
Gecko-6e22321
Gaia-1752e9e

STR
Create a new message 
Add to numbers or contacts into the To field
Add an attachment
Send the message
Once sent, open it and tap on the header
Once the recipient's details view is shown, go back tapping on '<' button

EXPECTED
User is taken back normally

ACTUAL
It is possible to go back without problems, but the back button '<' in the message view after coming back from the recipient's details view is shown as already pressed.
Please see screenshot attached.

Comment 1

[isabelrios]

Attachment: 1980-01-09-16-48-44.png

Comment 2

[Julien Wajsberg [:julienw]]

Hey Vivien,

this is a regression of your :focus patch. How should we fix this?

Comment 3

[J Zimbrick]

Regression Window:

Last Working Environmental Variables:
Device: Buri v1.3 Mozilla RIL
BuildID: 20131018040217
Gaia: 18c2ab4b7b2d4cacee90fb85e97354142b2fad4a
Gecko: 855da6d8a327
Version: 27.0a1
Base Image: V1.2_20131115

First Broken Environmental Variables:
Device: Buri v1.3 Mozilla RIL
BuildID: 20131019040204
Gaia: d0861c2913a57cb681ec4cd4d52d73cc0f479ac9
Gecko: e25e62d174ed
Version: 27.0a1
Base Image: V1.2_20131115

Comment 4

[Joe Cheng [:jcheng] (please needinfo)]

triage: regression 1.3+

Comment 5

[Julien Wajsberg [:julienw]]

Discussed with Vivien, we'll just override the :hover style on the back button and it should just work.

Comment 6

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #5)
> Discussed with Vivien, we'll just override the :hover style on the back
> button and it should just work.

Hi Julien/Vivien,
Since gecko patch in bug 863702 already landed too, do we still need hover patch in bug 921824? Another question is the element seems kept in hover state even after touchend. Is this a correct behavior? It might be annoying for element has been set hover styling in building block for solving the active state issue, but they have to override the hover state if there is no state change after touches.

Comment 7

[Julien Wajsberg [:julienw]]

> Another question is the element seems kept in hover state even after touchend. Is this a correct behavior?

Yep this is the behavior on all mobile platforms. This is especially useful for CSS-only menus that use :hover to display the subitems, and that we find a lot in the web.

I'll let Vivien answer the other question.

Comment 8

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

(In reply to Steve Chung [:steveck] from comment #6)
> (In reply to Julien Wajsberg [:julienw] from comment #5)
> > Discussed with Vivien, we'll just override the :hover style on the back
> > button and it should just work.
> 
> Hi Julien/Vivien,
> Since gecko patch in bug 863702 already landed too, do we still need hover
> patch in bug 921824? Another question is the element seems kept in hover
> state even after touchend. Is this a correct behavior? It might be annoying
> for element has been set hover styling in building block for solving the
> active state issue, but they have to override the hover state if there is no
> state change after touches.

The issue is that :hover is intended when the page will be removed from the screen. Then when the user touch something else the state should be removed. It seems like you are using back in a different way ?

Comment 9

[Steve Chung [:steveck]]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #8) 
> The issue is that :hover is intended when the page will be removed from the
> screen. Then when the user touch something else the state should be removed.
> It seems like you are using back in a different way ?

Yes, we don't refresh the header while switchinh these 2 views, here we only switch the content in the subpanel below. Overriding the hover style will having another problem that original active state will become unable to display in some scenario... I will find another solution for it(or any other suggestion to clear the hover state in JS would be helpful), thanks.

Comment 10

[Steve Chung [:steveck]]

Another important thing is the structure of this view in master is different from v1.3. Any changes of this view or even only this back button will cause conflict, which mean it might not a simple uplift task from master to v1.3. I will try to find a simple solution(overriding the button css seems not working...) workable for both master and 1.3, but it still posible that become 2 different solutions for 2 branches.

Comment 11

[Steve Chung [:steveck]]

Attachment: hover_css.patch

Here is the hover css overriding patch. It seems work except one bug: when enter the multiple recipient view and back to thread ui view, if you click the back button directly, active style is not applied. I'm not sure if it's a gecko bug or not, but it seems a more acceptable result then original one.

Hi Julien, should we create another gecko bug for it and land this css only patch first, or find another solution(BTW I could not find any JS solution to remove the hover status... :/) for it?

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 8361132 [details] [diff] [review]
hover_css.patch

Review of attachment 8361132 [details] [diff] [review]:
-----------------------------------------------------------------

The real fix is to have another header element for the information views.

Let's land this (with my comment) to unblock 1.3 but can you file another bug to have another header element for the information views in master ?

::: apps/sms/style/sms.css
@@ +1059,5 @@
>  }
> +
> +section[role="region"] > header:first-child > a:not([aria-disabled="true"]):hover:after {
> +  background-color: #F97C17 !important;
> +}

arg this is so ugly :(

to fix your remaining bug, you can add a new rule with the selector:

  section[role="region"] > header:first-child > a:not([aria-disabled="true"]):hover:active:after

Comment 13

[Steve Chung [:steveck]]

Attachment: Link to github

Thanks for the suggestion, will create another bug for master later and having another button in the information view header is also the only solution that comes to my mind.

Comment 14

[Julien Wajsberg [:julienw]]

Comment on attachment 8362343 [details] [review]
Link to github

r=me with a comment change

Comment 15

[Steve Chung [:steveck]]

Patch merged with comment addressed.
Landed in master: 27e7019096c13319b54158d4e10340ac161c3c9c

Comment 16

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 27e7019096c13319b54158d4e10340ac161c3c9c to:
v1.3: 61e86eb31fb2923c177604345fdf04b3215962a6

Comment 17

[isabelrios]

(In reply to Steve Chung [:steveck] from comment #13)
> Created attachment 8362343 [details] [review]
> Link to github
> 
> Thanks for the suggestion, will create another bug for master later and
> having another button in the information view header is also the only
> solution that comes to my mind.

Hi Steve, did you file that bug for master? Could you please share the number? Thanks!

Comment 18

[Steve Chung [:steveck]]

(In reply to Isabel Rios [:isabel_rios] from comment #17)
> (In reply to Steve Chung [:steveck] from comment #13)
> > Created attachment 8362343 [details] [review]
> > Link to github
> > 
> > Thanks for the suggestion, will create another bug for master later and
> > having another button in the information view header is also the only
> > solution that comes to my mind.
> 
> Hi Steve, did you file that bug for master? Could you please share the
> number? Thanks!

Hi Isabel, it is bug 961572.

Comment 19

[isabelrios]

With today's builds (01/24)
On v1.3 this bug is solved:
Gecko-d387d39
Gaia-0a79a06

But for master, 
Gecko-85b8bb0
Gaia-4699d23
The back icon '<' has been changed for the close one 'x', there is already a bug for that: Bug 963109.

Comment 20

[Steve Chung [:steveck]]

We change the back button icon image on purpose, but we didn't uplift this changes in v1.3. Please see the attachment 8344654 [details] and we should wait for Jose's comment in Bug 963109.

Comment 21

[isabelrios]

Hi Steve,
We know, but after checking with Ayman (UX) we opened bug 963109 as that back button should not be changed by close one on that screen. Anyway, lets wait for Jose's comment.
Thanks

Comment 22

[isabelrios]

On that screen I mean on Multi-recipient participants view, not the Message report one, where the close button is right

Comment 23

[Loli (:lolimartinezcr)]

Tested and working
Gecko ce6783c
Gaia 7306709