| Summary: | change MediaControlToggleClosedCaptionsButtonElement type to checkbox | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Danilo de Paula <danilo.eu> | ||||||||||||||||||||||||
| Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||
| Status: | RESOLVED WONTFIX | ||||||||||||||||||||||||||
| Severity: | Normal | CC: | buildbot, commit-queue, eric.carlson, esprehn+autocc, glenn, jer.noble, rniwa | ||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||||
| Bug Blocks: | 117649 | ||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||
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? 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). (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. (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. (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. Created attachment 204709 [details]
Patch
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 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 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
Created attachment 204719 [details]
Patch
(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. Comment on attachment 204719 [details]
Patch
Concept seems good, but patch doesn’t apply. Can you rebase it?
Created attachment 204721 [details]
Patch
Created attachment 204725 [details]
Patch
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 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
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 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
Created attachment 204814 [details]
Patch
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 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
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 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
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
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. |
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.