Closed
Bug 1385934
Opened 7 years ago
Closed 7 years ago
[RTL] Topsites in Activity Stream is right-aligned: should be center-aligned
Categories
(Firefox for Android Graveyard :: General, defect, P1)
Firefox for Android Graveyard
General
Tracking
(firefox57 fixed)
VERIFIED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: mcomella, Assigned: mcomella)
References
Details
(Whiteboard: [mobileAS])
Attachments
(4 files)
No description provided.
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Updated•7 years ago
|
Whiteboard: [mobileAS]
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → michael.l.comella
Assignee | ||
Updated•7 years ago
|
Iteration: --- → 1.28
Priority: P2 → P1
Assignee | ||
Comment 1•7 years ago
|
||
The top sites panel looks fine on launch.
The highlights introduction is incorrect and causes the page to scroll quite far - bug 1337425.
Assignee | ||
Comment 2•7 years ago
|
||
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.
Assignee | ||
Comment 3•7 years ago
|
||
(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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
There's another issue here- the Top Sites tiles at the top are aligned to the right, but should be aligned to the center.
Assignee | ||
Comment 5•7 years ago
|
||
(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...
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Assignee | ||
Comment 6•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Summary: [RTL] Verify Topsites in Activity Stream works in RTL → [RTL] Topsites in Activity Stream is right-aligned: should be center-aligned
(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.
Updated•7 years ago
|
Iteration: 1.28 → 1.29
Assignee | ||
Comment 8•7 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8902483 [details]
Bug 1385934: Add license to activity_stream_topsites_page.xml.
https://reviewboard.mozilla.org/r/174066/#review179320
Attachment #8902483 -
Flags: review?(liuche) → review+
Comment 12•7 years ago
|
||
mozreview-review |
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 :)
Attachment #8902484 -
Flags: review?(liuche) → review+
Comment 13•7 years ago
|
||
Pushed by michael.l.comella@gmail.com:
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9bfb38ac67b2
https://hg.mozilla.org/mozilla-central/rev/72ffe7066fc0
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•