Closed
Bug 769642
Opened 14 years ago
Closed 14 years ago
Reader Mode: Hide toolbar while loading content
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 16
People
(Reporter: lucasr, Assigned: lucasr)
References
Details
Attachments
(1 file)
|
3.10 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
Toolbar actions should not be available while reader content is being loaded.
|
Assignee | |
Comment 1•14 years ago
|
||
Attachment #637884 -
Flags: review?(mark.finkle)
|
|
Assignee | |
Updated•14 years ago
|
Summary: Reader Mode: Hide toolbar while loading contnet → Reader Mode: Hide toolbar while loading content
|
||
Comment 2•14 years ago
|
||
Comment on attachment 637884 [details] [diff] [review]
Only show toolbar once the reader content is shown
Patch looks fine, but I had a question about some previous code
>diff --git a/mobile/android/chrome/content/aboutReader.js b/mobile/android/chrome/content/aboutReader.js
> _setToolbarVisibility: function Reader_setToolbarVisibility(visible) {
> if (visible)
> toolbarElement.display = "block";
>
> toolbarElement.classList.toggle("toolbar-hidden");
Why do we need this check: toolbarElement.display = "block";
This should be enough: toolbarElement.classList.toggle(...);
and even so, we are missing the ".style" property, right?
r+ but let's address the old code
Attachment #637884 -
Flags: review?(mark.finkle) → review+
|
|
Assignee | |
Comment 3•14 years ago
|
||
(In reply to Mark Finkle (:mfinkle) from comment #2)
> Comment on attachment 637884 [details] [diff] [review]
> Only show toolbar once the reader content is shown
>
> Patch looks fine, but I had a question about some previous code
>
> >diff --git a/mobile/android/chrome/content/aboutReader.js b/mobile/android/chrome/content/aboutReader.js
>
> > _setToolbarVisibility: function Reader_setToolbarVisibility(visible) {
>
> > if (visible)
> > toolbarElement.display = "block";
> >
> > toolbarElement.classList.toggle("toolbar-hidden");
>
> Why do we need this check: toolbarElement.display = "block";
>
> This should be enough: toolbarElement.classList.toggle(...);
>
> and even so, we are missing the ".style" property, right?
>
> r+ but let's address the old code
This code is removed in my patch for bug 766961.
|
|
Assignee | |
Comment 4•14 years ago
|
||
|
||
Comment 5•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
|
||
Updated•5 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
•