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

RESOLVED FIXED in Firefox 54

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: tim.pan, Mentored)

Tracking

({good-first-bug})

unspecified
Firefox 54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

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

Attachments

(4 attachments)

Posted image Screenshot
See the screenshot.
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
Keywords: good-first-bug
Priority: -- → P3
Whiteboard: [good first bug][lang=css]
Assignee

Comment 2

2 years ago
I'd like to work on it. Its my first bug, so give me some time.
(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.
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)
(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

2 years 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.
I Think this issue is not present with nightly(54.0) Linux. I am attaching the screenshot.
(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.
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

2 years 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)
ok cool, I will look for another bug!
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+
Assignee: nobody → tim.pan

Comment 15

2 years 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e3cc6b9b36b3
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54

Comment 17

2 years ago
Posted image 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

2 years 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.