Closed Bug 701771 Opened 8 years ago Closed 8 years ago

Context Click menu shows FS option even within FS mode

Categories

(Firefox for Android :: General, defect, trivial)

ARM
Android
defect
Not set
trivial

Tracking

()

VERIFIED FIXED

People

(Reporter: tchung, Assigned: Margaret)

References

()

Details

(Whiteboard: [testday-20111111])

Attachments

(2 files)

Log into full screen video mode, and notice that context click menu still gives the option for full screen.  

See screenshot

Repro:
1) install fennec nightly, 20111111 build
2) visit a site with full screen video (see URL)
3) context click the video sample at the top, and enter full screen
4) once in FS, context click again, and verify the option is still available

Expected:
- FS mode should hide the FS context click option
Assignee: nobody → margaret.leibovic
Summary: Context Click menu shows FS option even within FS movde → Context Click menu shows FS option even within FS mode
Attached patch patchSplinter Review
We'll have to add a new selector when we implement the controls in bug 701785 since we want them to show up regardless of full screen state, but I think this fix makes sense for now since this is the only video context menu item we have right now.
Attachment #573933 - Flags: review?(wjohnston)
Comment on attachment 573933 [details] [diff] [review]
patch

Review of attachment 573933 [details] [diff] [review]:
-----------------------------------------------------------------

I think I'd rather we actually just not cache this Selector. i.e. My original plan was basically to only cache things that I thought we or extensions might need easy access too for more than a few menuitems. That is a fairly vague and nebulous set, but for this one, can we just do something like:

this.add(Strings.browser.GetStringFromName("contextmenu.fullScreen"),
  this.SelectorContext("video:not(:-moz-full-screen)"),
  function(aTarget) {
    aTarget.mozRequestFullScreen();
  }
);
Attachment #573933 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #3)
> Comment on attachment 573933 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> Review of attachment 573933 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> I think I'd rather we actually just not cache this Selector. i.e. My
> original plan was basically to only cache things that I thought we or
> extensions might need easy access too for more than a few menuitems. That is
> a fairly vague and nebulous set, but for this one, can we just do something
> like:
> 
> this.add(Strings.browser.GetStringFromName("contextmenu.fullScreen"),
>   this.SelectorContext("video:not(:-moz-full-screen)"),
>   function(aTarget) {
>     aTarget.mozRequestFullScreen();
>   }
> );

That makes sense - I was just following the model of the other Selectors :)
https://hg.mozilla.org/projects/birch/rev/a28cb9b7b7f4
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
20111114041052
http://hg.mozilla.org/projects/birch/rev/859ecdfe0168
Samsung Galaxy SII (Android 2.3.4)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.