Photon download toolbar icon progressbar should progress from right to left on RTL builds

VERIFIED FIXED in Firefox 57

Status

()

P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: itiel_yn8, Assigned: sfoster)

Tracking

(Blocks: 1 bug, {rtl})

unspecified
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [reserve-photon-animation])

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Currently also on RTL builds, in-progress downloads are horizontally progressing from left to right, this should be changed so it would progress the other way.
Whiteboard: [photon-animation] [triage]
Posted image Screenshot

Updated

2 years ago
Priority: -- → P1
Flags: qe-verify+
Priority: P1 → P3
QA Contact: jwilliams
Whiteboard: [photon-animation] [triage] → [reserve-photon-animation]
Comment hidden (mozreview-request)
Assignee: nobody → sfoster
Status: NEW → ASSIGNED
Iteration: --- → 57.1 - Aug 15
Priority: P3 → P1

Comment 4

2 years ago
mozreview-review
Comment on attachment 8893224 [details]
Bug 1386800 - Reverse the download progress bar & fix start animation in RTL locales.

https://reviewboard.mozilla.org/r/164266/#review170274

Do we need an LTR version also for indicatorArrowProgressDark?

Also, why would we need to reverse the start animation?
Attachment #8893224 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 5

2 years ago
(In reply to :Paolo Amadini from comment #4)
> Comment on attachment 8893224 [details]
> Bug 1386800 - Reverse the download progress bar & animation in RTL locales.
> 
> https://reviewboard.mozilla.org/r/164266/#review170274
> 
> Do we need an LTR version also for indicatorArrowProgressDark?

We don't currently use indicatorArrowProgressDark in Photon, instead the plan is to provide a different color for --toolbarbutton-icon-fill-attention in bug 1373341. 

> Also, why would we need to reverse the start animation?

An RTL document direction breaks the (new) start animation, and the coordinates need reversing. For reasons I hope to better understand today.

Comment 6

2 years ago
mozreview-review
Comment on attachment 8893224 [details]
Bug 1386800 - Reverse the download progress bar & fix start animation in RTL locales.

https://reviewboard.mozilla.org/r/164266/#review170350
Attachment #8893224 - Flags: review+

Comment 7

2 years ago
(In reply to Sam Foster [:sfoster] from comment #5)
> An RTL document direction breaks the (new) start animation, and the
> coordinates need reversing. For reasons I hope to better understand today.

We should probably find a better solution for the start animation, but if that's difficult we can land this workaround, as long as it's tested and working. This issue might apply to other animations as well, so it's good to investigate it regardless.

Can we file a bug to remove the code and %ifdef statements for non-Photon animations?
(Assignee)

Comment 8

2 years ago
(In reply to :Paolo Amadini from comment #7)

> We should probably find a better solution for the start animation, but if
> that's difficult we can land this workaround, as long as it's tested and
> working. This issue might apply to other animations as well, so it's good to
> investigate it regardless.

We used a similar solution in other animations, e.g. 

* http://searchfox.org/mozilla-central/source/browser/themes/shared/toolbarbutton-icons.inc.css#61
* http://searchfox.org/mozilla-central/source/browser/themes/shared/toolbarbutton-icons.inc.css#343

I'll update the patch to use scaleX(-1) in the same way. 

> Can we file a bug to remove the code and %ifdef statements for non-Photon
> animations?

Filed bug 1387530
Comment hidden (mozreview-request)
(Assignee)

Comment 10

2 years ago
Ok, I finally dredged some of this back up from brain/notes when I was working on RTL in FxOS. And cross-checked with what I would expect to see in HTML vs. XUL. So we do need the RTL variant for the progressbar animation - until a time when margin-inline-start/end are supported as animatable properties. For the start notification, that is just dropping vertically with no directionality, so simply setting an explicit direction of ltr maintains the animation in both LTR and RTL.

Comment 11

2 years ago
hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 2 changesets with 1 changes to 1 files (+1 heads)
remote: autoland push detected
remote: recorded push in pushlog
remote: 
remote: View your changes here:
remote:   https://hg.mozilla.org/try/rev/ef29c172a976c18b53a18c57ed10fa1b4c404cc0
remote:   https://hg.mozilla.org/try/rev/6af7bc4f65d241eae04624a0a32f50870031274d
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=6af7bc4f65d241eae04624a0a32f50870031274d
remote: recorded changegroup in replication log in 0.050s
remote: ** unknown exception encountered, please report by visiting
remote: ** https://mercurial-scm.org/wiki/BugTracker
remote: ** Python 2.7.5 (default, Nov  6 2016, 00:28:07) [GCC 4.8.5 20150623 (Red Hat 4.8.5-11)]
remote: ** Mercurial Distributed SCM (version 4.2.2)
remote: ** Extensions loaded: blackbox, clonebundles, obsolescencehacks, pushlog, serverlog, readonly, vcsreplicator
remote: Traceback (most recent call last):
remote:   File "/var/hg/venv_pash/bin/hg", line 45, in <module>
remote:     mercurial.dispatch.run()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 83, in run
remote:     status = (dispatch(req) or 0) & 255
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 164, in dispatch
remote:     ret = _runcatch(req)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 295, in _runcatch
remote:     return _callcatch(ui, _runcatchfunc)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 303, in _callcatch
remote:     return scmutil.callcatch(ui, func)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/scmutil.py", line 146, in callcatch
remote:     return func()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 285, in _runcatchfunc
remote:     return _dispatch(req)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 912, in _dispatch
remote:     cmdpats, cmdoptions)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 648, in runcommand
remote:     ret = _runcommand(ui, options, cmd, d)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 920, in _runcommand
remote:     return cmdfunc()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/dispatch.py", line 909, in <lambda>
remote:     d = lambda: util.checksignature(func)(ui, *args, **strcmdopt)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/util.py", line 1080, in check
remote:     return func(*args, **kwargs)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/commands.py", line 4663, in serve
remote:     s.serve_forever()
remote:   File "/var/hg/version-control-tools/hgext/serverlog/__init__.py", line 320, in serve_forever
remote:     return super(sshserverwrapped, self).serve_forever()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/sshserver.py", line 104, in serve_forever
remote:     while self.serve_one():
remote:   File "/var/hg/version-control-tools/hgext/serverlog/__init__.py", line 351, in serve_one
remote:     return super(sshserverwrapped, self).serve_one()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/sshserver.py", line 122, in serve_one
remote:     rsp = wireproto.dispatch(self.repo, self, cmd)
remote:   File "/var/hg/version-control-tools/hgext/serverlog/__init__.py", line 343, in dispatch
remote:     return origdispatch(repo, proto, cmd)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/extensions.py", line 231, in closure
remote:     return func(*(args + a), **kw)
remote:   File "/var/hg/version-control-tools/pylib/vcsreplicator/vcsreplicator/hgext.py", line 359, in wireprotodispatch
remote:     return orig(repo, proto, command)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/wireproto.py", line 570, in dispatch
remote:     return func(repo, proto, *args)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/wireproto.py", line 982, in unbundle
remote:     proto._client())
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/exchange.py", line 1777, in unbundle
remote:     lockandtr[2].close()
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/transaction.py", line 43, in _active
remote:     return func(self, *args, **kwds)
remote:   File "/var/hg/venv_pash/lib/python2.7/site-packages/mercurial/transaction.py", line 490, in close
remote:     self._postclosecallback[cat](self)
remote:   File "/var/hg/version-control-tools/hgext/pushlog/__init__.py", line 245, in onpostclose
remote:     conn.commit()
remote: sqlite3.OperationalError: database is locked
abort: stream ended unexpectedly (got 0 bytes, expected 4)

Comment 12

2 years ago
Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/16e248610f48
Reverse the download progress bar & fix start animation in RTL locales. r=Paolo

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/16e248610f48
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(Reporter)

Updated

2 years ago
Depends on: 1388432
(Assignee)

Updated

2 years ago
No longer depends on: 1388432
https://hg.mozilla.org/projects/date/rev/16e248610f483ae8dc3cb1d6fadffe07c501ac83
Bug 1386800 - Reverse the download progress bar & fix start animation in RTL locales. r=Paolo

Comment 15

2 years ago
I have reproduced this bug with Nightly 57.0a1 (ar) (2017-08-02) on Windows 8.1 , 64 Bit ! 

This bug's fix is Verified with latest Nightly (ar) !

Build   ID    20170811100330
User Agent    Mozilla/5.0 (Windows NT 6.3; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0
QA Whiteboard: [bugday-20170809]
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.