1019546 - Dialpad and Contacts are overlapped when moving from one to the other

Status: VERIFIED FIXED

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

Description

[Loli (:lolimartinezcr)]

Attachment: 2014-06-03-07-27-32.png

Tested
2.0
Hamachi
Gecko-c5087e3
Gaia-5dfe4ec

Reproducible 100%

Pre-requisites: Not contact added
STRs:
1. Tap dialer application.
2. Tap contact button.
3. Tap keypad button.

Actual result:
See attached screen. Two screen together.

Expected result:
Keypad screen showed

Comment 1

[Maria Angeles Oteo (:oteo)]

You can check the videos to see the regression from v1.4 to master:

Master: https://www.youtube.com/watch?v=h3wrI2fl2Pk

v1.4: https://www.youtube.com/watch?v=aUfwDX7daL8

v1.4 Build:
hamachi
B-80
Gecko-080d2fc
Gaia-ba8d7ef

Master build:
hamachi
B-185
Gecko-3e5340d
Gaia-e508b51

Comment 2

[Maria Angeles Oteo (:oteo)]

(In reply to Maria Angeles Oteo (:oteo) from comment #1)
> You can check the videos to see the regression from v1.4 to master:
> 

I made a mistake with the URLs of the links, including now the correct ones:

Master: https://www.youtube.com/watch?v=WsKVDGFZ4nE

v1.4: https://www.youtube.com/watch?v=aUfwDX7daL8

Comment 3

[Doug Sherk (:drs) (inactive)]

I bisected this down to this commit:
https://github.com/mozilla-b2g/gaia/commit/26d8fcab9b61f46451600f39c51e0387ef3c4f88

Comment 4

[Pi Wei Cheng [:piwei] (inactive)]

I can confirm the above comment is correct.

b2g-inbound Regression Window:

Last Working Environmental Variables:
Device: Buri
BuildID: 20140529103851
Gaia: c535744a3944cf466fc2d4060dbd2b35c4a09de6
Gecko: a1b5c50d2695
Version:  32.0a1
Firmware Version: v1.2device.cfg

First Broken Environmental Variables:
Device: Buri
Build ID: 20140529112650
Gaia: 26d8fcab9b61f46451600f39c51e0387ef3c4f88
Gecko: 8630b86e15c7
Version: 32.0a1
Firmware Version: v1.2device.cfg

Last Working Gaia / First Broken Gecko: Issue Does NOT reproduce
Gaia: c535744a3944cf466fc2d4060dbd2b35c4a09de6
Gecko: 8630b86e15c7

Last Working Gecko / First Broken Gaia: Issue DOES reproduce
Gaia: 26d8fcab9b61f46451600f39c51e0387ef3c4f88
Gecko: a1b5c50d2695

Gaia Pushlog:
https://github.com/mozilla-b2g/gaia/compare/c535744a3944cf466fc2d4060dbd2b35c4a09de6...26d8fcab9b61f46451600f39c51e0387ef3c4f88

Comment 5

[Doug Sherk (:drs) (inactive)]

Attachment: Fix dial pad and contacts overlapping when being switched between.

PR: https://github.com/mozilla-b2g/gaia/pull/20256

This fixes the issue, and I know why, but I'm not sure why it was ever a problem. It seems that we weren't blitting a background color on the keypad, so when switching to another view, we were momentarily having that view applied on top of this one.

As for why that was happening, I'm not really sure. This could actually be a graphics bug after all. One thing of note is that we're using hashes to switch between views, so maybe there's some kind of bug with that.

Filed bug 1022952 as a followup.

Comment 6

[Anthony Ricaud (:rik)]

Comment on attachment 8437276 [details] [diff] [review]
Fix dial pad and contacts overlapping when being switched between.

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

Thanks for filing the follow up bug!

Comment 7

[Doug Sherk (:drs) (inactive)]

master: https://github.com/mozilla-b2g/gaia/commit/c0b942c20e9b9d887401ffc19eac8b3d8c7dd30f
v2.0: https://github.com/mozilla-b2g/gaia/commit/dd3b9f32a60746366c8b6b5118fa2a556e5d73a9

Comment 8

[Loli (:lolimartinezcr)]

Tested and working
Hamachi
2.0
Gecko: 6558278
Gaia: B368142

2.1
Gecko-1ce168a
Gaia-207cacb

Comment 10

[Anthony Ricaud (:rik)]

This has caused bug 1019546. Can we backout dd3b9f32a60746366c8b6b5118fa2a556e5d73a9 and c0b942c20e9b9d887401ffc19eac8b3d8c7dd30f please?

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/163c6aec16e666b415409b4a1862121cf758b5eb
v2.0:   https://github.com/mozilla-b2g/gaia/commit/2d82cc9aec4cef4edb9f659c7da3dd4b849cba5b

Which by the way is way we typically wait to uplift until after a patch is confirmed to have stuck on master. Our landing rules even say the same ;)
https://wiki.mozilla.org/Release_Management/B2G_Landing

Comment 12

[Doug Sherk (:drs) (inactive)]

Attachment: Fix dialpad and contacts overlapping when moving from one to the other.

PR: https://github.com/mozilla-b2g/gaia/pull/20580

I decided to figure out what the actual issue was instead of just hacking a fix for it, and I ended up hacking a fix for it anyways. What's happening is we're setting the |aria-selected| attribute to false after the new view has been loaded, so we get some momentary clipping. The solution is to not rely on |aria-selected| for visibility, but unfortunately this code comes from BB. A better solution is to switch entirely to BB's tabs implementation, so I filed bug 1026079 as a followup.

We'll be able to close off bug 1022952 after landing this, as we know the cause now, and it's not a graphics bug after all.

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #11)
> Master:
> https://github.com/mozilla-b2g/gaia/commit/
> 163c6aec16e666b415409b4a1862121cf758b5eb
> v2.0:  
> https://github.com/mozilla-b2g/gaia/commit/
> 2d82cc9aec4cef4edb9f659c7da3dd4b849cba5b
> 
> Which by the way is way we typically wait to uplift until after a patch is
> confirmed to have stuck on master. Our landing rules even say the same ;)
> https://wiki.mozilla.org/Release_Management/B2G_Landing

Sorry, I got so used to my patches never rebasing cleanly that I just subconsciously started doing the rebases myself. :/

Comment 13

[Anthony Ricaud (:rik)]

Comment on attachment 8440857 [details] [diff] [review]
Fix dialpad and contacts overlapping when moving from one to the other.

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

We are using BB tabs, we include shared/style/tabs.css. We are changing all aria-selected attributes in the same loop so I don't understand why a loaded view would appear. If it has aria-selected=false, it shouldn't be displayed. Why is it?

Also, those selectors are way too complex, we want to rely as much as posible on classes and simple combinators in new code (OOCSS principle that I need to describe in a document)

Comment 14

[Doug Sherk (:drs) (inactive)]

(In reply to Anthony Ricaud (:rik) from comment #13)
> We are using BB tabs, we include shared/style/tabs.css. We are changing all
> aria-selected attributes in the same loop so I don't understand why a loaded
> view would appear. If it has aria-selected=false, it shouldn't be displayed.
> Why is it?

The |aria-selected| attribute is not what's showing and hiding the tabs. Actually, if I remove the call to set this, and all others like it, the issue is fixed. But if |aria-selected| is set, the side effect is that this tab's visibility changes.

What's happening is the hash change is taking effect before we get the callback for it. Between that time, we momentarily have |aria-selected=true| for one tab while the hash change is trying to hide it.

This has always been a problem, but it's been masked by our use of opaque background colors for views.

> Also, those selectors are way too complex, we want to rely as much as
> posible on classes and simple combinators in new code (OOCSS principle that
> I need to describe in a document)

They're just overrides for the BB selectors. I didn't write these myself. If I don't write them in this way, I can't disable one while keeping the other (due to specificity). The thing is, in BB [2], we have two selectors that apply to the same element for us. One of them gets applied too late, and the other too early.

I think it makes more sense to fix this in a followup by switching completely to BB. I don't think we would have this problem if we implemented hash changing using BB.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/dialer.js#L81
[2] https://github.com/mozilla-b2g/gaia/blob/master/shared/style/tabs.css#L196

Comment 15

[Anthony Ricaud (:rik)]

(In reply to Doug Sherk (:drs) from comment #14)
> (In reply to Anthony Ricaud (:rik) from comment #13)
> I think it makes more sense to fix this in a followup by switching
> completely to BB. I don't think we would have this problem if we implemented
> hash changing using BB.
It feels to me that BB should not use aria-selected for styling.

Arnau, Yura: You introduced this in bug 969553, can we safely remove this selector?

Comment 16

[Anthony Ricaud (:rik)]

See comment 15.

Comment 17

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

I will give it a try tomorrow with Paco ;)

Comment 18

[Anthony Ricaud (:rik)]

We discussed with Arnau over IRC. Removing this from Building blocks obviously means testing if other apps rely on it or not.

Comment 19

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Github PR

We need to ensure the accessibility helper is present when switching to contacts. It is not on the first tap, after that the styling works fine because it is checked for when switching to other tabs.

Comment 20

[Yura Zenevich [:yzen] (please use "needinfo")]

On the side note, aria-selected="true" is the only thing indicating which tab is selected. It could of course be something else - a class for example. But the above issue is caused by this JS error:

JavaScript error: http://communications.gaiamobile.org:8080/dialer/js/dialer.js, line 99: AccessibilityHelper is not defined

which as it turns results in aria-selected not set properly on contacts tab.

Comment 21

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8442805 [details] [review]
Github PR

As per IRC chat with Rik, asking for some feedback too.

Comment 22

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8442805 [details] [review]
Github PR

Yeah, I noticed this too. I'm not sure if this is meant to fix the issue or not, but it doesn't. Either way, it's something we should take, and I can sneak it into my patch.

Comment 23

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Anthony Ricaud (:rik) from comment #18)
> We discussed with Arnau over IRC. Removing this from Building blocks
> obviously means testing if other apps rely on it or not.

There will be a regression in, at least, the clock app, where the :target selector does not work. 

As a general case the selector will not work anywhere where the possible :target selectors are not contained in .bb-tablist.

One thing that worked for me is changing the way aria-selected is applied. Instead of iterating on the list of tabs and applying false for all but one, I tried setting all to false first and then setting the one selected with setTimeout of 1ms. It seems to work then but such 'fix' smells.

Comment 24

[Doug Sherk (:drs) (inactive)]

(In reply to Yura Zenevich [:yzen] from comment #23)
> One thing that worked for me is changing the way aria-selected is applied.
> Instead of iterating on the list of tabs and applying false for all but one,
> I tried setting all to false first and then setting the one selected with
> setTimeout of 1ms. It seems to work then but such 'fix' smells.

Personally, I don't like either solution, but I'd rather override the CSS for now.

Comment 25

[Anthony Ricaud (:rik)]

Comment on attachment 8442805 [details] [review]
Github PR

Redirecting the review since I'll be on PTO.

For the real fix, if we can't fix the BB now, let's take Doug's first approach and move to the web component version in the other bug.

Comment 26

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8440857 [details] [diff] [review]
Fix dialpad and contacts overlapping when moving from one to the other.

Redirecting to Etienne since Anthony didn't give a clear r+.

Comment 27

[Doug Sherk (:drs) (inactive)]

Attachment: Ensure AccessibilityHelper is loaded before using it.

(In reply to Yura Zenevich [:yzen] from comment #19)
> Created attachment 8442805 [details] [review]
> Github PR
> 
> We need to ensure the accessibility helper is present when switching to
> contacts. It is not on the first tap, after that the styling works fine
> because it is checked for when switching to other tabs.

Thanks, but this isn't enough. I took this change and included an additional test. I added it to my PR as another commit.

Comment 28

[Etienne Segonzac (:etienne)]

Comment on attachment 8443734 [details] [diff] [review]
Ensure AccessibilityHelper is loaded before using it.

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

::: apps/communications/dialer/test/unit/dialer_test.js
@@ +556,5 @@
> +      loadSpy = this.sinon.spy(LazyLoader, 'load');
> +      this.sinon.spy(AccessibilityHelper, 'setAriaSelected');
> +
> +      NavbarManager.resourcesLoaded = false;
> +    });

can you add a teardown setting back the window.location.hash to what it was?
(always afraid of a weird ordering failure we might get on a specific test environment)

Comment 29

[Etienne Segonzac (:etienne)]

Comment on attachment 8440857 [details] [diff] [review]
Fix dialpad and contacts overlapping when moving from one to the other.

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

What a mess... (the bug not the patch)

Comment 30

[Doug Sherk (:drs) (inactive)]

https://github.com/mozilla-b2g/gaia/commit/4ad629f8e50587d0095fdb8eefaf9c021efc53c3

Both landed with the teardown added.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/136cf2029216de8e61a7d5e337f59f90315c903c

Comment 32

[Loli (:lolimartinezcr)]

Tested and working
2.0
Gecko-c91e987
Gaia-87d0463

2.1
Gecko-330cda2
Gaia-756f1ae