1319584 - Remove right-border-radius

Status: NEW

(Toolkit :: Video/Audio Controls, defect, P3)

Description

[Justin Dolske [:Dolske]]

Attachment: Screenshot

http://searchfox.org/mozilla-central/rev/59bb309e38b10aba63dea8505fb800e99fe821d6/toolkit/themes/shared/media/videocontrols.css#239

When the position/volume scrubber is towards the left, the border-radius on the progress bar connects to the thumb in a strange way (ie, the radius is visible). As the thumb is moved towards the right, this issues gradually goes away. Which makes for a strange effect as you're dragging it.

See screenshot.

I think removing the border-bottom-right-radius and border-top-right-radius should fix this?

Comment 1

[Ray Lin[:ralin]]

Attachment: Bug 1319584 - remove strange radius between progress bar and thumb on video control.

Review commit: https://reviewboard.mozilla.org/r/95514/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95514/

Comment 2

[Ray Lin[:ralin]]

Attachment: shrink-width-screenshot.png

(In reply to Justin Dolske [:Dolske] from comment #0)
> Created attachment 8813437 [details]
> Screenshot
> 
> http://searchfox.org/mozilla-central/rev/
> 59bb309e38b10aba63dea8505fb800e99fe821d6/toolkit/themes/shared/media/
> videocontrols.css#239
> 
> When the position/volume scrubber is towards the left, the border-radius on
> the progress bar connects to the thumb in a strange way (ie, the radius is
> visible). As the thumb is moved towards the right, this issues gradually
> goes away. Which makes for a strange effect as you're dragging it.
> 
> See screenshot.
> 
> I think removing the border-bottom-right-radius and border-top-right-radius
> should fix this?

Thanks Justin! I've tried removing bottom-right and top-right radius, it worked but that would need extra flag to handle the style(state) since we still need the radius back when moving toward the end of progress.

I guess that shrinking the width of progress bar can also eliminate the gap. So I shrank the width by 2.5px on each side, and let thumb a tiny bit overhang on the edges. Could you give me feedback about the screenshot? or any concern about this approach? Thanks :)

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8814280 [details]
Bug 1319584 - remove strange radius between progress bar and thumb on video control.

https://reviewboard.mozilla.org/r/95514/#review96044

::: toolkit/themes/shared/media/videocontrols.css:228
(Diff revision 1)
>  .progressBar,
>  .scrubber,
>  .volumeBackground,
>  .volumeControl {
>    bottom: 0;
>    left: 0;
>    position: absolute;
>    width: 100%;
>    height: 100%;
>    padding: 0;
>    border: 0;
>    border-radius: 2.5px;

Shouldn't we just remove the border-top-right-radius and border-bottom-right-radius for .progressBar? And then test for RTL to see if we need to remove the border-*-left-radius rules too.

Comment 4

[Ray Lin[:ralin]]

Attachment: remove-right-radius-thumb.png

applied these lines of css: 

.progressBar,
.progressBar::-moz-progress-bar {
  border-top-right-radius: 0;
  border-bottom-right-radius: 0;
}

and the strange radius is hardly to be seen now. Also RTL doesn't reverse the direction of slider, so no need to remove the border-*-left-radius here.

A small problem is that progress bar protrudes from thumb when thumb moves to the end of slider(see the last screenshot). Therefore, I think we still need a flag to watch the progress, and add radius back at a right timing.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

If the video playback position is at 100%, we shouldn't be showing any part of a slider to the right of the thumb, so it looks like we need to make a slight change to either how the thumb gets positioned or remove some extra padding from the slider (I didn't investigate to see the reason why this is happening).

Comment 6

[Justin Dolske [:Dolske]]

(In reply to Ray Lin[:ralin] from comment #4)

> A small problem is that progress bar protrudes from thumb when thumb moves
> to the end of slider(see the last screenshot). Therefore, I think we still
> need a flag to watch the progress, and add radius back at a right timing.

Bah. Yeah, just removing the right-side border-radius isn't going to be enough. (And, if you look carefully at the top image in your screenshot, this doesn't even fully fix the issue when the scrubber is at the left. There's a slight gap where the top/bottom edge of the bar doesn't extend under the thumb.)

ISTR running into this kind of thing before. The core issue is that the input's thumb basically doesn't have a _precise_, fixed indicating point/pixel across the width of the control. One might expect the center pixel of the thumb to indicate the precise location, but it's not. At 0% it's the leftmost pixel (because it's flush-left, not overhanging the input's box), at 100% it's the rightmost pixel, and it changes in between. This makes perfect sense from a layout engine point-of-view (one box sliding wholly within another box), but leads to oddities like these. [A more visual demonstration is to just make the thumb semitransparent, and watch where the progress bar ends at.]

I think your first patch was on the right general track... Seems like we should either make the <input> slightly larger (1 thumb-width), or make the progressBackgroundBar slightly smaller (same), so that the end effect is the left and right edges of the <progress> are under the _center_ of the thumb when it's at the far left/right (and, therefore, all points in between).

Seems like this should be as simple as adding "margin: 0px 6.5px" (since the thumb is 13px) to the .progressBackgroundBar rule. That works as expect for the left side, but on the right he bar extends too far _past_ the thumb. O_o Why is nothing simple in CSS. :)

Also, note that all these issues apply to the volume slider too. So whatever fix we do should be done for both controls.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Hi Ray Lin, are you able to pick this back up with the details from comment 6?

Comment 8

[Ray Lin[:ralin]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> Hi Ray Lin, are you able to pick this back up with the details from comment
> 6?

I've fixed the issue on scrubber, and will push a WIP mozreview later. 
 
>Also, note that all these issues apply to the volume slider too. So whatever fix we do should be done for both controls.
I break accessibility test when applying this part. Volume control doesn't stack upon <progress>s but is only a <input type="range">, so it not feasible to shrink its backgroundBar as scrubber does. So here I need to adjust its structure but inevitably change accessible tree.

Comment 9

[Ray Lin[:ralin]]

Attachment: Bug 1319584 - [WIP] remove strange radius between progress bar and thumb on video control.

Review commit: https://reviewboard.mozilla.org/r/99730/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99730/

Comment 10

[Ray Lin[:ralin]]

Comment on attachment 8820201 [details]
Bug 1319584 - [WIP] remove strange radius between progress bar and thumb on video control.

Hi Jared, 
I've tried margin approach per comment 6, however, it doesn't shrink as expected since the <input> and <progress> are separate positioned elements. Adding margin to <progress> actually make it longer. So, I guess that shrink the centered <progress>'s width could get the correct result we want.

Could you help me to see if the patch looks good on your computer? and I also wondering that how to eliminate the unwanted `section` node in accessible tree caused by <meter> element.

Thanks :)

Comment 11

[Justin Dolske [:Dolske]]

Comment on attachment 8814284 [details]
shrink-width-screenshot.png

(I think my feedback was comment 6)

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8820201 [details]
Bug 1319584 - [WIP] remove strange radius between progress bar and thumb on video control.

https://reviewboard.mozilla.org/r/99730/#review107600

I looked at this and didn't have any ideas as to fixing this besides changing how the slider is implemented like Dolske commented. Unless we changed the design to not have a border-radius, but the border-radius looks pretty nice here. We could try doing a 1px or 2px border-radius to see how that looks.

It looks like the play button has a 2px border-radius at its corners, and the pause button has maybe a 5px border-radius, so if we changed the progress bar to have a 2px border-radius, we should look at tweaking the pause icon too.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

This bug is still valid. The issue is that while the thumb moves linearly through the progress of the playback, the progress bar (and likely the buffer bar) move at at a rate that is non-linear. This creates the problem where at the beginning of the video playback, the progress bar end is at the beginning of the thumb and at the end of the video playback the progress bar end is at the end of the thumb. The thumb should always be centered directly over the position of the progress bar end and they should move together consistently.