| Summary: | iOS8 media controls look different from iOS7 media controls | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||||||||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | calvaris, commit-queue, eric.carlson, glenn, jer.noble, jonlee, philipj, sergio, thorton | ||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Wenson Hsieh
2014-06-04 11:46:20 PDT
Created attachment 232489 [details]
Patch
(In reply to comment #0) > iOS8 media controls look different from iOS7 media controls <rdar://problem/16127943> Comment on attachment 232489 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232489&action=review > Source/WebCore/ChangeLog:3 > + iOS8 media controls look different from iOS7 media controls This describes the problem but not the solution, so please include a description of what has changed. > Source/WebCore/ChangeLog:20 > + (audio): > + (::-webkit-media-controls): > + (audio::-webkit-media-controls): > + (audio::-webkit-media-controls-wireless-playback-picker-button): > + (audio::-webkit-media-controls-panel): > + (video::-webkit-media-controls-panel): > + (audio::-webkit-media-controls-fullscreen-button): > + (audio::-webkit-media-controls-time-remaining-display): > + (video::-webkit-media-controls-current-time-display): > + (video::-webkit-media-controls-time-remaining-display): I think it is helpful for people reading the ChangeLog later to have a description of what changed in each method/rule. > Source/WebCore/Modules/mediacontrols/mediaControlsiOS.css:61 > + /* padding-top: 10px; */ Why is this commented out? > LayoutTests/platform/ios-sim/media/audio-width.html:1 > +<p>Test that audio elements have a minimum width.<p> Why is this before the <head> element? > LayoutTests/platform/ios-sim/media/audio-width.html:10 > +<audio id="aud_1" type="audio/mp4" controls></audio> > +<audio id="aud_2" type="audio/mp4" controls></audio> Why is this in the <head> element? Please create a fully formed document. I think that indentation aids readability and will help others have to have to read and understand this test later, so please use it. > LayoutTests/platform/ios-sim/media/audio-width.html:23 > +var width1 = width2 = NaN; // default value causes failure WebKit style is to declare each variable on its own line, and to only include comments when they document something that is not immediately obvious. > LayoutTests/platform/ios-sim/media/audio-width.html:26 > + width1 = parseInt(window.getComputedStyle(player1).width); // Npx parsed as int N This comment doesn't add much value, you can leave it out. > LayoutTests/platform/ios-sim/media/audio-width.html:30 > + testRunner.notifyDone(); You include video-test.js so you can call endTest() instead and automatically get a logging that the test ended successfully. > LayoutTests/platform/ios-sim/media/audio-width.html:38 > +var player1 = document.getElementById("aud_1"), > + player2 = document.getElementById("aud_2"); > +if (player1 && player2) { > + player1.src = player2.src = "../../../media/" + findMediaFile("audio", "content/test"); > + runTest(); > +} 1 - Please put this in a function in <head> and call it in response to an event (body.load or some-such). 2 - Please declare each variable. 3 - Is it likely that player1 or player2 will ever be null? Comment on attachment 232489 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232489&action=review Thanks, uploading a new patch soon. Sorry for the lack of descriptions/information and generally messy code -- this was a work in progress, and I'll have a more recent version out very soon. >> Source/WebCore/ChangeLog:3 >> + iOS8 media controls look different from iOS7 media controls > > This describes the problem but not the solution, so please include a description of what has changed. Expanded to include brief description of fixes. >> Source/WebCore/ChangeLog:20 >> + (video::-webkit-media-controls-time-remaining-display): > > I think it is helpful for people reading the ChangeLog later to have a description of what changed in each method/rule. More detailed descriptions of changes added. >> Source/WebCore/Modules/mediacontrols/mediaControlsiOS.css:61 >> + /* padding-top: 10px; */ > > Why is this commented out? Sorry about that -- removed altogether >> LayoutTests/platform/ios-sim/media/audio-width.html:1 >> +<p>Test that audio elements have a minimum width.<p> > > Why is this before the <head> element? Fixed. >> LayoutTests/platform/ios-sim/media/audio-width.html:10 >> +<audio id="aud_2" type="audio/mp4" controls></audio> > > Why is this in the <head> element? Please create a fully formed document. > > I think that indentation aids readability and will help others have to have to read and understand this test later, so please use it. Fixed and added indentation to the file, thanks for the tip >> LayoutTests/platform/ios-sim/media/audio-width.html:23 >> +var width1 = width2 = NaN; // default value causes failure > > WebKit style is to declare each variable on its own line, and to only include comments when they document something that is not immediately obvious. Fixed. >> LayoutTests/platform/ios-sim/media/audio-width.html:26 >> + width1 = parseInt(window.getComputedStyle(player1).width); // Npx parsed as int N > > This comment doesn't add much value, you can leave it out. Fixed. >> LayoutTests/platform/ios-sim/media/audio-width.html:30 >> + testRunner.notifyDone(); > > You include video-test.js so you can call endTest() instead and automatically get a logging that the test ended successfully. Got it - fixed. >> LayoutTests/platform/ios-sim/media/audio-width.html:38 >> +} > > 1 - Please put this in a function in <head> and call it in response to an event (body.load or some-such). > 2 - Please declare each variable. > 3 - Is it likely that player1 or player2 will ever be null? Removed null check and restructured javascript. Created attachment 232586 [details]
Patch
Please ignore the css comment at line 326 -- that should've been deleted. Sorry! Created attachment 232589 [details]
Patch
Comment on attachment 232589 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232589&action=review > Source/WebCore/ChangeLog:3 > + iOS8 media controls look different from iOS7 media controls. Adjusted CSS to make media elements more similar to iOS7 by adding padding, adjusting hues/svg, resizing elements, etc. Please reword as discussed on IRC. > Source/WebCore/Modules/mediacontrols/mediaControlsiOS.css:88 > + background-color: rgb(0, 0, 0); You could use the "black" keyword here. > Source/WebCore/Modules/mediacontrols/mediaControlsiOS.css:102 > + -webkit-mask-image: url('data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 42 44"><g fill="none"><path stroke="white" stroke-width="1" d="M15,27.5 10.5,27.5 10.5,14.5 30.5,14.5 30.5,27.5 26,27.5 30.5,27.5 30.5,14.5 10.5,14.5 10.5,27.5 15,27.5 Z" /><path fill="white" d="M20.5,23.5 13,31.5 28,31.5" /></g></svg>'); There are more spaces between path data elements in this one and the one above than the others. Intentional? > LayoutTests/platform/ios-sim/media/audio-width.html:1 > + There's an extra newline here Comment on attachment 232589 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232589&action=review >> Source/WebCore/ChangeLog:3 >> + iOS8 media controls look different from iOS7 media controls. Adjusted CSS to make media elements more similar to iOS7 by adding padding, adjusting hues/svg, resizing elements, etc. > > Please reword as discussed on IRC. Thanks for the heads up. Fixed. Comment on attachment 232589 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232589&action=review Thanks for taking a look at this! -Wenson >> Source/WebCore/Modules/mediacontrols/mediaControlsiOS.css:88 >> + background-color: rgb(0, 0, 0); > > You could use the "black" keyword here. Good catch. Fixed. >> Source/WebCore/Modules/mediacontrols/mediaControlsiOS.css:102 >> + -webkit-mask-image: url('data:image/svg+xml,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 42 44"><g fill="none"><path stroke="white" stroke-width="1" d="M15,27.5 10.5,27.5 10.5,14.5 30.5,14.5 30.5,27.5 26,27.5 30.5,27.5 30.5,14.5 10.5,14.5 10.5,27.5 15,27.5 Z" /><path fill="white" d="M20.5,23.5 13,31.5 28,31.5" /></g></svg>'); > > There are more spaces between path data elements in this one and the one above than the others. Intentional? It was intentional, for the purpose of readability (the other paths were generally simple enough that reading the points wasn't too difficult) >> LayoutTests/platform/ios-sim/media/audio-width.html:1 >> + > > There's an extra newline here Fixed. Thanks! Comment on attachment 232589 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232589&action=review > Source/WebCore/Modules/mediacontrols/mediaControlsiOS.css:26 > + min-width: 260px; Nit: odd spacing, is there a tab in here? > LayoutTests/platform/ios-sim/media/audio-width.html:30 > + console.log("testing..."); console.log() doesn't end up in the results. > LayoutTests/platform/ios-sim/media/audio-width.html:33 > + var audioPath = "../../../media/" + findMediaFile("audio", "content/test"); But: I think you can include the "../../" in the path passed to findMediaFile. > LayoutTests/platform/ios-sim/media/audio-width.html:36 > + runTest(); Nit: this doesn't necessarily have to be a separate function. If you update the ChangeLog and upload the new patch without orphaning the old one, any committee can mark it cq+ Created attachment 232599 [details]
Patch
Comment on attachment 232589 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232589&action=review Thanks! There will be another update in a few minutes. >> Source/WebCore/Modules/mediacontrols/mediaControlsiOS.css:26 >> + min-width: 260px; > > Nit: odd spacing, is there a tab in here? hmm...I'm not sure I see the tab >> LayoutTests/platform/ios-sim/media/audio-width.html:30 >> + console.log("testing..."); > > console.log() doesn't end up in the results. Oops...forgot to take out some debugging information. Good catch. >> LayoutTests/platform/ios-sim/media/audio-width.html:33 >> + var audioPath = "../../../media/" + findMediaFile("audio", "content/test"); > > But: I think you can include the "../../" in the path passed to findMediaFile. got it. Fixed. >> LayoutTests/platform/ios-sim/media/audio-width.html:36 >> + runTest(); > > Nit: this doesn't necessarily have to be a separate function. Contents of runTest moved here. Created attachment 232600 [details]
Patch
(In reply to comment #15) > Created an attachment (id=232600) [details] > Patch Did you upload the correct version of the patch? (In reply to comment #16) > (In reply to comment #15) > > Created an attachment (id=232600) [details] [details] > > Patch > > Did you upload the correct version of the patch? (In reply to comment #16) > (In reply to comment #15) > > Created an attachment (id=232600) [details] [details] > > Patch > > Did you upload the correct version of the patch? Hm..that's strange. I think some of the changes didn't make it on. Created attachment 232601 [details]
Patch
(In reply to comment #18) > Created an attachment (id=232601) [details] > Patch Oh, no wonder; the test .html file I was editing was a backup version I saved in a tmp directory >.< That explains it... Hopefully this new one has everything. Comment on attachment 232601 [details] Patch Rejecting attachment 232601 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 232601, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Tim Thorton found in /Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog does not appear to be a valid reviewer according to contributors.json. /Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://webkit-queues.appspot.com/results/5904323871506432 Created attachment 232602 [details]
Patch for upload
Comment on attachment 232602 [details] Patch for upload Rejecting attachment 232602 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 232602, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Full output: http://webkit-queues.appspot.com/results/4926571345346560 Created attachment 232603 [details]
Patch for landing
Comment on attachment 232603 [details] Patch for landing Clearing flags on attachment: 232603 Committed r169636: <http://trac.webkit.org/changeset/169636> |