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

Review please? Thanks

Comment on attachment 8449956 [details] 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.

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 on attachment 8449956 [details] 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 on attachment 8449956 [details] 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.

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

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

(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.

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

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.

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

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!

(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..

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

Comment on attachment 8520718 [details] 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 on attachment 8520718 [details] Github pull-request Thanks Ahmed - looks good!

(In reply to Kevin Grandon :kgrandon (In Europe/Conf until 11/24) from comment #17) > Comment on attachment 8520718 [details] > 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] > Github pull-request > > Thanks Ahmed - looks good! Thanks! And so sorry for late..

(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.

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 on attachment 8520718 [details] 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.

(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.

(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)

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.

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?

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.

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

RTL is out of 2.2.

(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 on attachment 8540871 [details] [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 on attachment 8540871 [details] [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 on attachment 8540871 [details] [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 on attachment 8540871 [details] [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 on attachment 8540871 [details] [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 on attachment 8540871 [details] [PullReq] anefzaoui:bug-1033917-1 to mozilla-b2g:master Reassigning ui-review? to Francis since this pertains to Rocketbar.

Comment on attachment 8540871 [details] [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!

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 on attachment 8540871 [details] [PullReq] anefzaoui:bug-1033917-1 to mozilla-b2g:master Done, filed Bug 1118713 for the progress bar. How does it look now? Thanks!

Comment on attachment 8540871 [details] [PullReq] anefzaoui:bug-1033917-1 to mozilla-b2g:master I am comfortable landing this, thank you for your work!

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

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

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

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