1385934 - [RTL] Topsites in Activity Stream is right-aligned: should be center-aligned

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Screenshot on launch

The top sites panel looks fine on launch.

The highlights introduction is incorrect and causes the page to scroll quite far - bug 1337425.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Screenshot: top sites full of data

Top sites looks fine full and correctly scrolls for more options from right to left.

However, the highlights items is not correct:
- the page domain text should be right-aligned
- "Visited" should be right-aligned to the icon.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #2)
> However, the highlights items is not correct:

Filed bug 1393274 so we can keep this bug and verify that we verified the top sites panel RTL already.

Comment 4

[Itiel]

There's another issue here- the Top Sites tiles at the top are aligned to the right, but should be aligned to the center.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to ItielMaN from comment #4)
> There's another issue here- the Top Sites tiles at the top are aligned to
> the right, but should be aligned to the center.

Thanks for pointing that out! That is weird...

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

It looks like we're using a third-party RtlViewPager [1] for those items. We imported it in 2017 [2], which is after the latest commit to the repository (2016) [3]. There are no open issues for this bug (but it's a small project - 72 stars & 20 forks) so it's possible that this is an error in our code.

[1]: https://github.com/diego-gomez-olvera/RtlViewPager
[2]: https://hg.mozilla.org/mozilla-central/log/tip/mobile/android/thirdparty/com/booking/rtlviewpager/RtlViewPager.java
[3]: https://github.com/diego-gomez-olvera/RtlViewPager/commits/master

Comment 7

[Itiel]

(In reply to Michael Comella (:mcomella) from comment #6)
> It looks like we're using a third-party RtlViewPager [1] for those items. We
> imported it in 2017 [2], which is after the latest commit to the repository
> (2016) [3]. There are no open issues for this bug (but it's a small project
> - 72 stars & 20 forks) so it's possible that this is an error in our code.

FWIW, I remember it worked okay on previous implementation of the Top Sites tiles.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

It looks like the problem is we specify `paddingLeft` in activity_stream_topsites_page.xml [1] which goes wrong when we switch to RTL. fwiw, the paddingEnd looks like it exists because of the margins between the views in the ViewPager (rather than an explicit margin that maybe have been specified).

[1]: http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/mobile/android/app/src/main/res/layout/activity_stream_topsites_page.xml#6

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1385934: Add license to activity_stream_topsites_page.xml.

Review commit: https://reviewboard.mozilla.org/r/174066/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174066/

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1385934: Use RTL layout attr in activity_stream_topsites_page.

Review commit: https://reviewboard.mozilla.org/r/174068/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174068/

Comment 11

[Chenxia Liu [:liuche]]

Comment on attachment 8902483 [details]
Bug 1385934: Add license to activity_stream_topsites_page.xml.

https://reviewboard.mozilla.org/r/174066/#review179320

Comment 12

[Chenxia Liu [:liuche]]

Comment on attachment 8902484 [details]
Bug 1385934: Use RTL layout attr in activity_stream_topsites_page.

https://reviewboard.mozilla.org/r/174068/#review179322

lgtm!

::: mobile/android/app/src/main/res/layout/activity_stream_topsites_page.xml:6
(Diff revision 1)
>  <?xml version="1.0" encoding="utf-8"?>
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<!-- The paddingEnd visible to the user comes from the padding between items. As such, paddingEnd here should be 0dp.

Helpful comment :)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9bfb38ac67b2
Add license to activity_stream_topsites_page.xml. r=liuche
https://hg.mozilla.org/integration/autoland/rev/72ffe7066fc0
Use RTL layout attr in activity_stream_topsites_page. r=liuche

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9bfb38ac67b2
https://hg.mozilla.org/mozilla-central/rev/72ffe7066fc0

Comment 15

[Itiel]

Confirmed fixed on latest Nightly.