903960 - [SMS] Switching to message view when selecting left side of the message

Status: RESOLVED FIXED

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

Description

[Leo]

1. Title: Switching to message view when selecting left side of the message 
2. Precondition: Should have some messages 
3. Tester's Action:  Message -> Edit messages -> touch on the left near the select button -> Check the screen
4. Detailed Symptom (ENG.) : The edit screen is moved to message view when slecting on the left of the button.
5. Expected:  The edit screen should not be moved to message view when selecting.
6. Reproducibility: Y
1) Frequency Rate : 100%
7. Gaia Master/v1-train: Reproduced on v1-train
8. Gaia Revision:   c5e8358d753b8954de7b1a8c3e00944617aeec5e
9. Personal email id: [e-mail]

Comment 1

[Leo]

Attachment: screenshot attached

Please find the attached screenshot for the issue.
1.The edit screen is moved to message view when slecting on the left of the button.
2.But, when selecting on the right side of the message at any place the message is selected.

Comment 2

[Julien Wajsberg [:julienw]]

Saw this recently too, feels weird.

Comment 3

[Evelyn Eastmond [:evhan55]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/12512

- Fix
    - Fixed by moving the `.pack-checkbox` `<li>` over to the left, to completely cover the underlying `<a>` tag that was launching the message view.  Then, compensated for the move by moving the contained `<span>` tag containing the checkbox graphic over to the right again
- Tests
    - Manual in both desktop and on Unagi device using `m-c` gecko
    - Need any automated tests?

Comment 4

[Julien Wajsberg [:julienw]]

(In reply to Evelyn Eastmond [:evhan55] from comment #3)

>     - Need any automated tests?

For this sort of stuff, automated tests needs to be integration tests. Do you feel like bootstrapping the framework on the sms app ?

Comment 5

[Evelyn Eastmond [:evhan55]]

Yes, sure thing, I'd like to learn how to write those.
Is there anywhere you suggest I look for guidance on how to get started?

(In reply to Julien Wajsberg [:julienw] from comment #4)
> (In reply to Evelyn Eastmond [:evhan55] from comment #3)
> 
> >     - Need any automated tests?
> 
> For this sort of stuff, automated tests needs to be integration tests. Do
> you feel like bootstrapping the framework on the sms app ?

Comment 6

[Julien Wajsberg [:julienw]]

Asking jugglinmike might be a good start if you happen to have him close to you ;)

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 811313 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12512

comments on github

The CSS is not simple in that area and I'd rather prevent the click in JS than piling up more spaghetti CSS here.

Thanks !

Comment 8

[Evelyn Eastmond [:evhan55]]

Comment on attachment 811313 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12512

I retried this time in JS and would like some feedback before I write the unit (or integration) test:

Using (evt.currentTarget == this.container) was too inclusive and prevented even the checkbox click.  I am not sure if the checks I am doing now are ok (checking against the hash of the <a> tag that we are interested in).

Also, this fix works in that it prevents clicking through to the message detail when clicking to the left of the checkbox, but that <a> tag still highlights blue on the press event, on both desktop and unagi device.  Should I investigate that blue highlight glitch as part of this patch?  I'm sure the highlight is purposefully there for regular clicks, but probably shouldn't appear on this prevented click.

Comment 9

[Evelyn Eastmond [:evhan55]]

Comment on attachment 811313 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12512

Hi Rick, wondering if you have time to give feedback on this before I write a unit test for it.

Comment 10

[Julien Wajsberg [:julienw]]

(In reply to Evelyn Eastmond [:evhan55] from comment #8)
> Comment on attachment 811313 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/12512
> 

> Using (evt.currentTarget == this.container) was too inclusive and prevented
> even the checkbox click.

Could you please share the code you tried, like on a specific branch ?

Are you sure you've put this check _after_ the call to checkInputs in handleEvent ?

Comment 11

[Evelyn Eastmond [:evhan55]]

(In reply to Julien Wajsberg [:julienw] (in Paris-Web conference thursday and friday) from comment #10)
> (In reply to Evelyn Eastmond [:evhan55] from comment #8)
> > Comment on attachment 811313 [details] [review]
> > https://github.com/mozilla-b2g/gaia/pull/12512
> > 
> 
> > Using (evt.currentTarget == this.container) was too inclusive and prevented
> > even the checkbox click.
> 
> Could you please share the code you tried, like on a specific branch ?
> 
> Are you sure you've put this check _after_ the call to checkInputs in
> handleEvent ?

Yes, I think I have put the check after the call to checkInputs, but perhaps I am doing it wrong.

I have put a branch with that code here:
https://github.com/evhan55/gaia/tree/903960-option1

When I run this branch, clicking on threads in the thread list edit mode has no effect, nothing happens.

Comment 12

[Evelyn Eastmond [:evhan55]]

(In reply to Julien Wajsberg [:julienw] (in Paris-Web conference thursday and friday) from comment #10)
> (In reply to Evelyn Eastmond [:evhan55] from comment #8)
> > Comment on attachment 811313 [details] [review]
> > https://github.com/mozilla-b2g/gaia/pull/12512
> > 
> 
> > Using (evt.currentTarget == this.container) was too inclusive and prevented
> > even the checkbox click.
> 
> Could you please share the code you tried, like on a specific branch ?
> 
> Are you sure you've put this check _after_ the call to checkInputs in
> handleEvent ?

I have uploaded the two options I tried here:
https://github.com/evhan55/gaia/compare/903960-option1

As you can see, evt.currentTarget == this.container no matter what type of click it is, so I think that check doesn't help.

The only difference I can see readily is that evt.target.hash exists when the click is on the href element.  That is why I tried using the existence of a hash as the solution.

Let me know!

Comment 13

[Evelyn Eastmond [:evhan55]]

Oops link: https://github.com/evhan55/gaia/tree/903960-option1

Comment 14

[Evelyn Eastmond [:evhan55]]

Please refer to
https://bugzilla.mozilla.org/show_bug.cgi?id=903960#c12
https://bugzilla.mozilla.org/show_bug.cgi?id=903960#c13

Comment 15

[Julien Wajsberg [:julienw]]

Discussed on IRC,

It now appears that the first CSS fix was indeed the good one, I feel very sorry about this.

This is imho a BB bug, so let's do a BB fix, and ask review from Sergi ([e-mail]) !

Comment 16

[Julien Wajsberg [:julienw]]

Quite sure this is _not_ a regression: the reporter reproduces on 1.1, and I really think this was like that on 1.0 too.

Comment 17

[Evelyn Eastmond [:evhan55]]

Comment on attachment 811313 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12512

Hi Julien,

I tried a new approach to this bug in CSS.  One change is in sms.css, the transform went over *too far* and exposed the underlying <a> tag.  So I fixed that transform.

However, then the checkbox was not placed correctly, it was positioned to be at left: 0px of the containing label, but we want that padding, so I placed it at 1rem.  This is potentially a BB change, I think.

Submitting to you for feedback.
Also, should this be tested with unit or integration test?
After we finally agree on how to fix and test this, I will then submit to Sergi for review like you said.
Unless you think I should mark him for feedback, too?

Evelyn

Comment 18

[Evelyn Eastmond [:evhan55]]

It's not clear to me what BB changes are OK to make and how to test them throughout the app.  I am feeling insecure about the CSS change in shared/style_unstable/lists.css.

Comment 19

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

triage: should not block release since this bug has been around for some time but please land it directly in master

Comment 20

[Julien Wajsberg [:julienw]]

Comment on attachment 811313 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12512

I think the label does not go far enough to the left: when we tap on the left, it should still check the checkbox.

However, we also want to retain the transform animation, where the checkbox comes from outside the window in the left. (granted this animation does not run on b2g right now, maybe because we have a reflow when we trigger the edit mode, but let's still not regress on this).

Maybe it's possible to not transform the label, but transform the fake checkbox only instead ? Therefore we may not need to change the BB ?

I agree that we should not change the BB with this patch, if one part of the fix is in the BB and another part, maybe we should keep all in sms.css. On the other hand, I think we're the only ones using these checkboxes ;-)

If my other idea doesn't work, we can ask Sergi who might have an idea too.

Comment 21

[Evelyn Eastmond [:evhan55]]

Comment on attachment 811313 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12512

Resubmitting for feedback before attempting a test:

- Fix
    - Added a margin to the checkbox
    - Changed the left position of the label to start further off to the left
- Tests
    - Bootstrap an integration test?

Comment 22

[Julien Wajsberg [:julienw]]

Comment on attachment 811313 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12512

I think this looks good and does what we want.
I'm still gonna ask a feedback from Sergi as he sees the big picture with building blocks.

Question: have you tried to use the transform+transition on the span (that holds the fake checkbox) instead of the label? Just so that we know all the possible solutions.

About integration tests, I would be very grateful to see a bootstrap from someone that already did some in another app. ;)

Comment 23

[Julien Wajsberg [:julienw]]

Also, we see some transition overlap when going out of edit mode (I see it more easily on the device than in Firefox Desktop), you might want to tune the various transitions we have.

Comment 24

[Evelyn Eastmond [:evhan55]]

> Question: have you tried to use the transform+transition on the span (that
> holds the fake checkbox) instead of the label? Just so that we know all the
> possible solutions.

Here is a branch that tries it this way instead, and I like it better:
https://github.com/evhan55/gaia/compare/903960-span-transform

It doesn't have the overlap issue on the 'hide' that you found and seems to work pretty well.
What do you think?


> About integration tests, I would be very grateful to see a bootstrap from
> someone that already did some in another app. ;)

Ok, I will ask around to see how much effort it might be for me to do it properly.
If it takes significant time and effort, I may have to ask for help.

Comment 25

[Evelyn Eastmond [:evhan55]]

Flagging NI for Julien for comment #24

Comment 26

[Julien Wajsberg [:julienw]]

Seeing the code only, I like it too. I won't have the time to try on a device today, and Monday is a day off in France, but maybe you can prepare a PR with this and ask a review from Steve (:steveck).

Comment 27

[Evelyn Eastmond [:evhan55]]

Comment on attachment 811313 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12512

Resubmitting for review:

- Fix
    - Changed the left position of the label to take up the whole space
    - Removed the translation from the label
    - Added translation to the span with the checkbox itself
- Tests
    - Bootstrap an integration test?

Comment 28

[Evelyn Eastmond [:evhan55]]

(In reply to Julien Wajsberg [:julienw] from comment #26)
> Seeing the code only, I like it too. I won't have the time to try on a
> device today, and Monday is a day off in France, but maybe you can prepare a
> PR with this and ask a review from Steve (:steveck).

Sounds good, thank you

Comment 29

[Steve Chung [:steveck]]

Comment on attachment 811313 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12512

Hi Evelyn, some comments here: https://github.com/mozilla-b2g/gaia/pull/12512/files#r7553949. Just some small concern and I'm ok with the solution. About the integration test, it would be greate if someone can start the sms app part, but I will not block this patch for integration test requirement. Seems we already created a bug 930556 for test, but I would like to treat it as meta bug and we can create seperated bugs(test cases) for different message app modules.

Comment 30

[Evelyn Eastmond [:evhan55]]

(In reply to Steve Chung [:steveck] from comment #29)
> Comment on attachment 811313 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/12512
> 
> Hi Evelyn, some comments here:
> https://github.com/mozilla-b2g/gaia/pull/12512/files#0. Just some small
> concern and I'm ok with the solution. 

Resubmitting for review:

Hi Steve, I addressed the concerns regarding the transition order and application to both the label and the span.  I also fixed another small glitch that had to do with the <p> label not transitioning gracefully out of edit state, which is related to this bug.  Now, the label stays in place, and both the span and the label seem to transition in and out correctly.

- Fix
    - Changed the left position of the label to take up the whole space
    - Removed the translation and translation transition from the `<label>`
    - Added translation and transition to the `<span>` with the checkbox itself
    - Separated the translation transition for the `<p>` so it moves back gracefully

> About the integration test, it would
> be greate if someone can start the sms app part, but I will not block this
> patch for integration test requirement.

Ok, I have changed the tests section of my PR to say:

- Tests
    - TBD in another bug

Comment 31

[Julien Wajsberg [:julienw]]

Comment on attachment 811313 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12512

I'll let Steve finish this review alone, I'm sure he'll do it correctly :)

Comment 32

[Steve Chung [:steveck]]

Comment on attachment 811313 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12512

Hi Evelyn, some more feedback on the github(just some performance concern), thanks for the update!

Comment 33

[Evelyn Eastmond [:evhan55]]

It seems quite a lot has changed very recently in the underlying way these span tags are created, I think they are now psuedo elements?

https://github.com/mozilla-b2g/gaia/commit/59c5f36da1913f9834bcee3ef312575ca1be2077#diff-1094651ffacfe36453a4e20abc2e9792

My fix is now out of date (it's behaving differently from the last time I ran it) and I have to re-look at the problem.

Evelyn

Comment 34

[Evelyn Eastmond [:evhan55]]

Comment on attachment 811313 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12512

Resubmitting for review.  Had to change the CSS selector based on some recent changes in style_unstable.

Comment 35

[Steve Chung [:steveck]]

Comment on attachment 811313 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/12512

It looks good now, thanks for the fixing!

Comment 36

[Steve Chung [:steveck]]

Merged in master: 778da780d3db0d6d3dac00caebeb8f56244b542e

Comment 37

[Evelyn Eastmond [:evhan55]]

(In reply to Steve Chung [:steveck] from comment #35)
> Comment on attachment 811313 [details] [review]
> https://github.com/mozilla-b2g/gaia/pull/12512
> 
> It looks good now, thanks for the fixing!

Thank you!