"Exit Full Screen" button on the video fullscreen UI isn't properly vertically aligned

RESOLVED FIXED in Firefox 54

Status

()

Firefox
Theme
P3
normal
RESOLVED FIXED
4 months ago
15 days ago

People

(Reporter: Ehsan, Assigned: Timothy Pan, Mentored)

Tracking

({good-first-bug})

unspecified
Firefox 54
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [good first bug][lang=css])

Attachments

(4 attachments)

Created attachment 8827537 [details]
Screenshot

See the screenshot.

Comment 1

4 months ago
Probably just need to remove the top and bottom margin from #fullscreen-exit-button in browser/themes/shared/fullscreen/warning.inc.css.
Mentor: dao+bmo@mozilla.com
Keywords: good-first-bug
Priority: -- → P3
Whiteboard: [good first bug][lang=css]
(Assignee)

Comment 2

4 months ago
I'd like to work on it. Its my first bug, so give me some time.

Comment 3

4 months ago
(In reply to Timothy Pan from comment #2)
> I'd like to work on it. Its my first bug, so give me some time.

It's not an urgent issue, so take your time! Let me know if you have questions.

Comment 4

4 months ago
Hey, I could not reproduce it in nightly 54.0. And I cannot even see the #fullscreen-exit-button in the specified path.
Flags: needinfo?(dao+bmo)

Comment 5

4 months ago
(In reply to Srivatsav Gunisetty from comment #4)
> Hey, I could not reproduce it in nightly 54.0.

What operating system are you using? I think this is more visible on macOS than on other platforms.

> And I cannot even see the
> #fullscreen-exit-button in the specified path.

You would need to add a new rule for that.
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 6

4 months ago
As far as I can tell it looks like the problem is with the rule "html|*.pointerlockfswarning-exit-button", because it does not to be applied at all. As you suggested once I changed the selector back to #fullscreen-exit-button everything worked, all the declarations including setting margins to 0 were applied. 

But since all of the other rules work and use a class selector, I suppose the best way to fix this would be to add the class to the button, instead of using id selector.

Comment 7

4 months ago
Created attachment 8833638 [details]
Screenshot_nightly(54.0)_linux.png

I Think this issue is not present with nightly(54.0) Linux. I am attaching the screenshot.

Comment 8

4 months ago
(In reply to Timothy Pan from comment #6)
> As far as I can tell it looks like the problem is with the rule
> "html|*.pointerlockfswarning-exit-button", because it does not to be applied
> at all. As you suggested once I changed the selector back to
> #fullscreen-exit-button everything worked, all the declarations including
> setting margins to 0 were applied. 

I'm not sure what the point of this obsolete rule is. Looks like nobody missed it except for the margin issue. I'd review a patch that updates the selector and strips down the rule to just removing the margin.

> But since all of the other rules work and use a class selector, I suppose
> the best way to fix this would be to add the class to the button, instead of
> using id selector.

This is because those elements occur twice in browser.xul. The button exists only once, though, so let's keep using the id.

Comment 9

3 months ago
hi, can i also work on this bug ? I see Timothy is also trying to work on it.
Timothy, do you want to keep working on this?
Flags: needinfo?(tim.pan)
(Assignee)

Comment 11

3 months ago
Yes, I can do it. I've just been a little busy. I just have to read the documentation on how to submit the fix properly.
Flags: needinfo?(tim.pan)

Comment 12

3 months ago
ok cool, I will look for another bug!
(Assignee)

Comment 13

3 months ago
Created attachment 8837214 [details] [diff] [review]
Fix CSS rule to select by ID and set margins to 0 only.
Comment on attachment 8837214 [details] [diff] [review]
Fix CSS rule to select by ID and set margins to 0 only.

Looks good. Thanks!
Attachment #8837214 - Flags: review+

Updated

3 months ago
Assignee: nobody → tim.pan

Comment 15

3 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3cc6b9b36b3
Change fs warning button css rule to use ID selector and remove margins. r=dao

Comment 16

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e3cc6b9b36b3
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54

Comment 17

2 months ago
Created attachment 8852642 [details]
attachment.png

While verifying the fix of this bug with Latest Nightly 55.0a1 (2017-03-29)on Windows 10 (64 bit), I felt that the "Exit Full Screen" button is not properly aligned.   
I have attached a screenshot of the fix that I saw.Is the current fix (attached in the attachment) should be expected as the fix?

[bugday-20170329]
QA Whiteboard: [good first verify]

Comment 18

15 days ago
I can still reproduce this issue on the latest nightly using Windows 10 x64.
You need to log in before you can comment on or make changes to this bug.