Closed Bug 1311700 Opened 3 years ago Closed 3 years ago

Add test to confirm that video control show controls in different sizes correctly

Categories

(Toolkit :: Video/Audio Controls, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: ralin, Assigned: ralin)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Add a UI test to confirm video control has correct layout under different sizes.

1. controls in controlbar should show/hide according to width of video region.
2. controls in controlbar should display size properly. The size of button should be fixed, and flexible slider should resize accordingly.
3. controlbar should hide when it can't not fit in <video>'s height.
Hi jaws,

Do you know how to get right(computed) size of each controls in mochitest? i.e. playButton.clientWidth // should return 30;
I've tried some API(clientWidth, getComputedStyle, getBoundingClientRect, etc.) and also wrapped the elements by SpecialPowers, but always got 0 instead of right size. Does different type of test matter(I currently run in plain mochitest)? I suspect that is because content process could not probe anonymous content correctly.

Thanks
Flags: needinfo?(jaws)
We use clientWidth in videocontrols.xml. As long as we have a reference to the right element clientWidth should work. Did you make sure that the video controls had appeared first? They will need to be visible to get layout to apply the styles and size the elements.
Flags: needinfo?(jaws)
Assignee: nobody → ralin
Status: NEW → ASSIGNED
Attachment #8809264 - Attachment is obsolete: true
Attachment #8809264 - Attachment is patch: false
Comment on attachment 8812639 [details]
Bug 1311700 - Part 1. add a test to confirm the correctness of video control in different sizes.

https://reviewboard.mozilla.org/r/94280/#review94770

::: toolkit/content/tests/widgets/test_videocontrols_size.html:45
(Diff revision 1)
> +  const minControlBarWidth = 48;
> +  const minClickToPlaySize = 48;
> +  const maxVolumeSliderWidth = 64;
> +
> +  function getElementName(elem) {
> +    return  elem.getAttribute("anonid") || elem.getAttribute("class");

nit, one space between 'return' and 'elem.getAttribute'

::: toolkit/content/tests/widgets/test_videocontrols_size.html:167
(Diff revision 1)
> +  function getElementWithinVideoByAttribute(video, aName, aValue) {
> +    const videoControl = domUtils.getChildrenForNode(video, true)[1];
> +
> +    return SpecialPowers.wrap(videoControl.ownerDocument)
> +      .getAnonymousElementByAttribute(videoControl, aName, aValue);
> +  }

This function should be moved to head.js and then removed from the other tests that also define it.
Attachment #8812639 - Flags: review?(jaws) → review+
Comment on attachment 8813961 [details]
Bug 1311700 - Part 2. move shared function to head.js.

https://reviewboard.mozilla.org/r/95268/#review96048
Attachment #8813961 - Flags: review?(jaws) → review+
Blocks: 1319301
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bdb32806a818
Part 1. add a test to confirm the correctness of video control in different sizes. r=jaws
https://hg.mozilla.org/integration/autoland/rev/fd9319f6ddce
Part 2. move shared function to head.js. r=jaws
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bdb32806a818
https://hg.mozilla.org/mozilla-central/rev/fd9319f6ddce
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.