Closed Bug 769642 Opened 10 years ago Closed 10 years ago

Reader Mode: Hide toolbar while loading content

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 16

People

(Reporter: lucasr, Assigned: lucasr)

References

Details

Attachments

(1 file)

Toolbar actions should not be available while reader content is being loaded.
Attachment #637884 - Flags: review?(mark.finkle)
Summary: Reader Mode: Hide toolbar while loading contnet → Reader Mode: Hide toolbar while loading content
Blocks: 766935
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+
(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.
https://hg.mozilla.org/mozilla-central/rev/55d0b0f169fc
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.