Bug 117177 – change MediaControlToggleClosedCaptionsButtonElement type to checkbox

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebCore Misc.

[Danilo de Paula]

Created attachment 203646 [details]
patch

it's a draft patch to change the type of the MediaControlToggleClosedCaptionsButtonElement from "button" to "checkbox"

For what I could see, all ports except Mac treats the MediaControlToggleClosedCaptionsButtonElement as a toggle button to show/hide captions/subtitles from html videos.
They even use setChecked(true/false) as a "state" for this button.
However the checked state is useless on a mediaControl[PORT].css point of view because the css selector ":checked" is not valid for <input type=button>

My propose here is to change the type of that button to checkbox and change the defaultEventHandler to get the changeEvent. 
That way, ports will be able to use that information by css and current implementations won't be hurt.

[Jer Noble]

There are many conditions where a request to enter full screen may be denied by browser chrome(). How does this patch handle the case where, after clicking the enter full screen button, the button should remain in the "non-full screen" state?

[Jer Noble]

Also, there seems to be no logic in this patch to uncheck the full screen button when the user exits full screen mode via another means (like the "ESC" key).

[Eric Carlson]

(In reply to comment #1)
> There are many conditions where a request to enter full screen may be denied by browser chrome(). How does this patch handle the case where, after clicking the enter full screen button, the button should remain in the "non-full screen" state?

(In reply to comment #2)
> Also, there seems to be no logic in this patch to uncheck the full screen button when the user exits full screen mode via another means (like the "ESC" key).

This patch proposes a change to the closed caption button, not the fullscreen button.

[Jer Noble]

(In reply to comment #3)
> (In reply to comment #1)
> > There are many conditions where a request to enter full screen may be denied by browser chrome(). How does this patch handle the case where, after clicking the enter full screen button, the button should remain in the "non-full screen" state?
> 
> (In reply to comment #2)
> > Also, there seems to be no logic in this patch to uncheck the full screen button when the user exits full screen mode via another means (like the "ESC" key).
> 
> This patch proposes a change to the closed caption button, not the fullscreen button.

Ack! I completely misread the button class.

[Danilo de Paula]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #1)
> > > There are many conditions where a request to enter full screen may be denied by browser chrome(). How does this patch handle the case where, after clicking the enter full screen button, the button should remain in the "non-full screen" state?
> > 
> > (In reply to comment #2)
> > > Also, there seems to be no logic in this patch to uncheck the full screen button when the user exits full screen mode via another means (like the "ESC" key).
> > 
> > This patch proposes a change to the closed caption button, not the fullscreen button.
> 
> Ack! I completely misread the button class.

Do you guys agree with my approach? If so, I'll write a proper patch with that change.

[Danilo de Paula]

Created attachment 204709 [details]
Patch

[Eric Carlson]

Comment on attachment 204709 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204709&action=review

> LayoutTests/media/video-controls-captions.html:78
> +            // the :checked state of the button should be accesible by css
> +

Nit: this comment is unnecessary, it just states the obvious

[Build Bot]

Comment on attachment 204709 [details]
Patch

Attachment 204709 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/918024

New failing tests:
media/video-controls-captions-trackmenu-hide-on-click.html
fast/repaint/table-cell-collapsed-border-scroll.html
media/video-controls-captions-trackmenu-hide-on-click-outside.html

[Build Bot]

Created attachment 204716 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Danilo de Paula]

Created attachment 204719 [details]
Patch

[Danilo de Paula]

(In reply to comment #9)
> Created an attachment (id=204716) [details]
> Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
> 
> The attached test failures were seen while running run-webkit-tests on the mac-ews.
> Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

Hit a failure on mac, looks like the tackList menu didn't work on Mac. I tried to find the problem and fix it, but I don't have a mac machine to see the final result.

[Darin Adler]

Comment on attachment 204719 [details]
Patch

Concept seems good, but patch doesn’t apply. Can you rebase it?

[Danilo de Paula]

Created attachment 204721 [details]
Patch

[Danilo de Paula]

Created attachment 204725 [details]
Patch

[Build Bot]

Comment on attachment 204725 [details]
Patch

Attachment 204725 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/921073

New failing tests:
fast/repaint/table-cell-collapsed-border-scroll.html
media/video-controls-captions-trackmenu-hide-on-click-outside.html

[Build Bot]

Created attachment 204751 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 204725 [details]
Patch

Attachment 204725 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/864122

New failing tests:
media/video-controls-captions-trackmenu-hide-on-click-outside.html

[Build Bot]

Created attachment 204756 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Danilo de Paula]

Created attachment 204814 [details]
Patch

[Build Bot]

Comment on attachment 204814 [details]
Patch

Attachment 204814 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/921327

New failing tests:
media/video-controls-captions-trackmenu-hide-on-click.html
fast/repaint/table-cell-collapsed-border-scroll.html
media/video-controls-captions-trackmenu-hide-on-click-outside.html

[Build Bot]

Created attachment 204833 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 204814 [details]
Patch

Attachment 204814 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/906408

New failing tests:
media/video-controls-captions-trackmenu-hide-on-click.html
media/video-controls-captions-trackmenu-hide-on-click-outside.html

[Build Bot]

Created attachment 204836 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Darin Adler]

Comment on attachment 204814 [details]
Patch

setting review- because of the EWS failure; seems like a good idea but still failing the tests on the Mac

[Danilo de Paula]

As I don't own a mac machine I've end up implementing the feature in GTK by using the same approach as sarafi does. I'm not working on it anymore.

I'm closing the bug as wontfix.