1033917 - [Rocketbar][RTL] Vertical Homescreen E.me search input should be right aligned in RTL locales

Status: VERIFIED FIXED

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

Description

[Ahmed Nefzaoui [:Nefzaoui]]

Search button should be on the left while the "search or enter address" equivalent should be right-aligned in RTL locales.

Comment 1

[Ahmed Nefzaoui [:Nefzaoui]]

Attachment: Screenshot of bug 1033917

Comment 2

[Ahmed Nefzaoui [:Nefzaoui]]

Attachment: Github Pull Request

Review please?
Thanks

Comment 3

[Kevin Grandon :kgrandon]

Comment on attachment 8449956 [details] [review]
Github Pull Request

Hey Ahmed! I think this patch is really great, and we can definitely use it. I do think that we should wait until we get the full solution for the search bar as well though, as clicking on it will cause the text to reverse sides and it seems quite jarring.

Would you happen to have time to investigate the search implementation as well? I think you basically have a R+ here, but I would like to land/see everything at once if possible.

Comment 4

[Ahmed Nefzaoui [:Nefzaoui]]

Both search bars are now rtl-compatible.

Kevin, could you please look at it and tell me if there's something else that needs to be done concerning this bug?
Thanks :)

Comment 5

[Ahmed Nefzaoui [:Nefzaoui]]

Comment on attachment 8449956 [details] [review]
Github Pull Request

I looked into both search bars and they both look consistent now, if that's what was your comment about..
Can you look into this, please? :)
Thanks

Comment 6

[Kevin Grandon :kgrandon]

Comment on attachment 8449956 [details] [review]
Github Pull Request

Hi Ahmed - thank you for your patch. We are very close here, but I think the padding on the "Search or enter address" is not quite right. There seems to be a large amount of padding to the right of the label, when the rightmost letter should be flush against the right side of the box I would assume?

Also there is going to be a *lot* of refactoring to the search bar in the 2.1 release, but we can certainly land this in the meantime if you'd like.

Comment 7

[Ahmed Nefzaoui [:Nefzaoui]]

No problem then, I'll look at it again once the refactoring lands.

Comment 9

[Stephany Wilkes]

Kevin, did this actually land in the meantime? I'd love to get this in even if other work supersedes it. Thanks!

Comment 10

[Kevin Grandon :kgrandon]

(In reply to Stephany Wilkes from comment #9)
> Kevin, did this actually land in the meantime? I'd love to get this in even
> if other work supersedes it. Thanks!

The initial pieces of the new rocketbar have landed, so we can certainly start landing this RTL work. The pieces have moved around though, so this will probably need a rebase.

Comment 11

[Stephany Wilkes]

Kevin, do you think we can rebase and land for 2.1?

Comment 12

[Kevin Grandon :kgrandon]

Yes it's definitely possible, but I'm swamped with things this week. If someone has cycles please go for it, otherwise I will start looking next week. Blocking rocketbar-next so we're tracking this.

Comment 13

[Stephany Wilkes]

Kevin, what's the status on this one? Thanks!

Comment 14

[Kevin Grandon :kgrandon]

I think we've done some adjustments here recently, but we need to take another pass at the overall Rocketbar RTL support. 

Ahmed - I would like to see if it makes sense to rebase this patch on top of master. Are you around, and is this something you could help us with? Thanks!

Comment 15

[Ahmed Nefzaoui [:Nefzaoui]]

(In reply to Kevin Grandon :kgrandon (In Europe/Conf until 11/24) from comment #14)
> I think we've done some adjustments here recently, but we need to take
> another pass at the overall Rocketbar RTL support. 
> 
> Ahmed - I would like to see if it makes sense to rebase this patch on top of
> master. Are you around, and is this something you could help us with? Thanks!

Hey
I think maybe I'll go with a new PR.
Looking into this now..

Comment 16

[Ahmed Nefzaoui [:Nefzaoui]]

Attachment: Github pull-request

Voilà :)
Sorry for late.. How does that one look?

Thanks :)

Comment 17

[Kevin Grandon :kgrandon]

Comment on attachment 8520718 [details] [review]
Github pull-request

Thanks Ahmed, this is great! There's a few problems with our browser chrome controls that I noticed - is this something you'd be able to help us with? I'll upload a screenshot of the problem now.

The code for this is quite messy TBH - do you think migrating to something like flexbox could help here?

Comment 18

[Kevin Grandon :kgrandon]

Attachment: Screenshot of browser chrome /w patch

Comment 19

[Stephany Wilkes]

Comment on attachment 8520718 [details] [review]
Github pull-request

Thanks Ahmed - looks good!

Comment 20

[Ahmed Nefzaoui [:Nefzaoui]]

(In reply to Kevin Grandon :kgrandon (In Europe/Conf until 11/24) from comment #17)
> Comment on attachment 8520718 [details] [review]
> Github pull-request
> 
> Thanks Ahmed, this is great! There's a few problems with our browser chrome
> controls that I noticed - is this something you'd be able to help us with?
> I'll upload a screenshot of the problem now.

Oh my.. I should have looked further into it. Sorry :/ And yes, of course, whatever it takes.
Tho, I'm not gonna be able to do much until 18th Oct. :/

> The code for this is quite messy TBH - do you think migrating to something
> like flexbox could help here?

You mean Rocketbar code, browser chrome code or this PR's, is messy?
And Yes, I'm in favor of flexbox :)

(In reply to Stephany Wilkes from comment #19)
> Comment on attachment 8520718 [details] [review]
> Github pull-request
> 
> Thanks Ahmed - looks good!

Thanks! And so sorry for late..

Comment 21

[Kevin Grandon :kgrandon]

(In reply to Ahmed Nefzaoui [:Nefzaoui] (Please needinfo? | Away from 30 OCT to 18 NOV) from comment #20)
> > The code for this is quite messy TBH - do you think migrating to something
> > like flexbox could help here?
> 
> You mean Rocketbar code, browser chrome code or this PR's, is messy?
> And Yes, I'm in favor of flexbox :)

Sorry, I was referring to the existing rocketbar code that handles the browser chrome elements. I wasn't sure how hard swapping the refresh and ssl lock icons would be - which I thought is something we'd want to do.

Comment 22

[Stephany Wilkes]

Wait - we'd need to spec that and put that in the bidi patterns doc, if so. I believe the refresh icon would stay the same, same place, and the SSL icon would also align right. Bumping to review- because I just  noticed that - sorry Ahmed, thanks Kevin!

Comment 23

[Stephany Wilkes]

Comment on attachment 8520718 [details] [review]
Github pull-request

Woops - moving to review- for SSL icon alignment. Also we should make sure the spinner/refresh and SSL icons have sufficient padding and don't obscure Rocketbar text.

Comment 24

[Kevin Grandon :kgrandon]

(In reply to Stephany Wilkes from comment #22)
> Wait - we'd need to spec that and put that in the bidi patterns doc, if so.
> I believe the refresh icon would stay the same, same place, and the SSL icon
> would also align right. Bumping to review- because I just  noticed that -
> sorry Ahmed, thanks Kevin!

It seems that we either need to keep the controls as-is for LTR, or swap the ordering. Doing that is somewhere between both, and not something we can easily do :)

If we are going to be swapping the direction for header elements across the OS, it makes sense to me that we would swap the direction of the chrome controls as well. If we're not swapping the direction of header elements (and I mean the buttons and text within headers), then I feel like the browser chrome should stay as-is.

Comment 25

[Ahmed Nefzaoui [:Nefzaoui]]

(In reply to Stephany Wilkes from comment #22)
> Wait - we'd need to spec that and put that in the bidi patterns doc, if so.
> I believe the refresh icon would stay the same, same place, and the SSL icon
> would also align right. Bumping to review- because I just  noticed that -
> sorry Ahmed, thanks Kevin!

No problem! However concerning the browser chrome controls, I believe this an acceptable order in RTL:

[...] [[X] [Page Title] [SSL]] [< (Forward button)] [> (Back button)]

Just saying :)

(In reply to Kevin Grandon :kgrandon (In Europe/Conf until 11/24) from comment #21)
> (In reply to Ahmed Nefzaoui [:Nefzaoui] (Please needinfo? | Away from 30 OCT
> to 18 NOV) from comment #20)
> > > The code for this is quite messy TBH - do you think migrating to something
> > > like flexbox could help here?
> > 
> > You mean Rocketbar code, browser chrome code or this PR's, is messy?
> > And Yes, I'm in favor of flexbox :)
> 
> Sorry, I was referring to the existing rocketbar code that handles the
> browser chrome elements. I wasn't sure how hard swapping the refresh and ssl
> lock icons would be - which I thought is something we'd want to do.

So then flexbox or not, it's always possible to swap them (as long as this doesn't include gaia-elements)

Comment 26

[Stephany Wilkes]

Ahmed, you ARE the expert here so I happily defer to you and what you would expect (and like) to see here. :) No need to change them now.

Comment 27

[Ahmed Nefzaoui [:Nefzaoui]]

I'd like to know when can I resume working on this :) Should I wait for something to be reimplemented or just fix the nits and carry on?

Comment 28

[Kevin Grandon :kgrandon]

Ahmed - if you have some cycles to help we could definitely use it :) This patch looked good, but there were just a few problems with the browser chrome. It looked like some of the controls weren't properly swapped when I tested the patch. For example - it looked like the "reload" button was on the wrong side of the chrome. Did you see that when you apply the patch? To see this - you'll need to load a webpage from rocketbar or the browser.

Comment 29

[Kevin Grandon :kgrandon]

Hey Ahmed - any thoughts on comment 28? Would you want to fix the remaining few issues for the chrome here?

Comment 30

[Kai-Chih Hu [:khu]]

RTL is out of 2.2.

Comment 31

[Ahmed Nefzaoui [:Nefzaoui]]

(In reply to Kevin Grandon :kgrandon from comment #29)
> Hey Ahmed - any thoughts on comment 28? Would you want to fix the remaining
> few issues for the chrome here?

Yes preferably here :)

(In reply to Kevin Grandon :kgrandon from comment #28)
> Ahmed - if you have some cycles to help we could definitely use it :) This
> patch looked good, but there were just a few problems with the browser
> chrome. It looked like some of the controls weren't properly swapped when I
> tested the patch. For example - it looked like the "reload" button was on
> the wrong side of the chrome. Did you see that when you apply the patch? To
> see this - you'll need to load a webpage from rocketbar or the browser.

On it :)

Comment 32

[GitHub Autolander Bot]

Attachment: [PullReq] anefzaoui:bug-1033917-1 to mozilla-b2g:master

Comment 33

[Ahmed Nefzaoui [:Nefzaoui]]

Comment on attachment 8540871 [details] [review]
[PullReq] anefzaoui:bug-1033917-1 to mozilla-b2g:master

Kevin and Stephany,
It's ready for review, and gaia-try is happy :)
Could you please review?

*UX Review notice *
This patch:
Fixes Homescreen Rochetbar search RTL layout.
Fixes Rocketbar Clicked-state of the search RTL layout.
Fixes browser chrome controls.
Fixes the issue of browser chrome progress bar.
Fixes Rocketbar problem of being initiated by clicking the left half of the status bar even when in RTL.

Thanks! :)

Comment 34

[Kevin Grandon :kgrandon]

Comment on attachment 8540871 [details] [review]
[PullReq] anefzaoui:bug-1033917-1 to mozilla-b2g:master

Thanks for the patch. I left some comments on github. Can you address and re-flag me? I think we're close!

Comment 35

[Ahmed Nefzaoui [:Nefzaoui]]

Comment on attachment 8540871 [details] [review]
[PullReq] anefzaoui:bug-1033917-1 to mozilla-b2g:master

Thanks Kevin!
I answered your questions and fixed the nits.
How does that look now?

Thanks!

Comment 36

[Kevin Grandon :kgrandon]

Comment on attachment 8540871 [details] [review]
[PullReq] anefzaoui:bug-1033917-1 to mozilla-b2g:master

Ok looking good. The last thing I noticed that I'd like to clean up is some code in the gaia_progress. I'd prefer to just not swap the direction as we're inconsistent all over the OS, but if you want to for now we can leave it it.

I left a comment about it on github, I'd like to clean up the usage and naming of init(). Please address and flag me when done. Thanks!

Comment 38

[Ahmed Nefzaoui [:Nefzaoui]]

Comment on attachment 8540871 [details] [review]
[PullReq] anefzaoui:bug-1033917-1 to mozilla-b2g:master

Hey Kevin,

How does it look now? Let me know if I forgot anything.

Thanks!

Comment 41

[Stephany Wilkes]

Comment on attachment 8540871 [details] [review]
[PullReq] anefzaoui:bug-1033917-1 to mozilla-b2g:master

Reassigning ui-review? to Francis since this pertains to Rocketbar.

Comment 42

[Kevin Grandon :kgrandon]

Comment on attachment 8540871 [details] [review]
[PullReq] anefzaoui:bug-1033917-1 to mozilla-b2g:master

Looks good. I left one comment about the progress element work, but other than that I think we are good to go. Go ahead and flag me for review again when ready. Thanks!

Comment 43

[Kevin Grandon :kgrandon]

Ahmed - Can you do me a favor here? After looking at this again I want to split out the browser chrome work from the progress bar. Can you submit a new pull request with only the the chrome work, leaving the progress bar alone?

I've seen some more issues with the progress bar, and I'm not really comfortable landing it without tests. I think we should move forward with landing the rest though, and doing the progress bar alone. TBH - I'm still not sure that we change the progress bar, I don't think that we have any studies that determine which way it should animate in an RTL setting, and our progress bars across the OS don't yet match. (The progress bar moves one direction in browser, and another in calendar for example).

Comment 44

[Ahmed Nefzaoui [:Nefzaoui]]

Comment on attachment 8540871 [details] [review]
[PullReq] anefzaoui:bug-1033917-1 to mozilla-b2g:master

Done, filed Bug 1118713 for the progress bar.
How does it look now?
Thanks!

Comment 45

[Kevin Grandon :kgrandon]

Comment on attachment 8540871 [details] [review]
[PullReq] anefzaoui:bug-1033917-1 to mozilla-b2g:master

I am comfortable landing this, thank you for your work!

Comment 46

[Kevin Grandon :kgrandon]

Most work here is in the system, so moving components, and adding checkin-needed for autolander.

Comment 47

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/be6ea020f4864c99801fb00a8d04f5c3a4726030

Comment 52

[Elie]

Attachment: 2015-01-15-05-37-16.png

This issue verified successfully on Flame 2.2:
Gaia-Rev        7c5b27cad370db377b18a742d3f3fdb0070e899f
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/ce27f2692382
Build-ID        20150115002505
Version         37.0a2

Comment 54

[Lancy(Leave from Mozilla)]

Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/15444/