Bug 133521

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:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch for upload	none
Patch for landing	none

Wenson Hsieh

Reported 2014-06-04 11:46:20 PDT

iOS8 media controls look different from iOS7 media controls

Attachments
Patch (7.40 KB, patch) 2014-06-04 11:51 PDT, Wenson Hsieh	no flags	Details Formatted Diff Diff
Patch (15.28 KB, patch) 2014-06-05 15:43 PDT, Wenson Hsieh	no flags	Details Formatted Diff Diff
Patch (15.10 KB, patch) 2014-06-05 16:10 PDT, Wenson Hsieh	no flags	Details Formatted Diff Diff
Patch (15.11 KB, patch) 2014-06-05 19:59 PDT, Wenson Hsieh	no flags	Details Formatted Diff Diff
Patch (15.11 KB, patch) 2014-06-05 20:13 PDT, Wenson Hsieh	no flags	Details Formatted Diff Diff
Patch (14.96 KB, patch) 2014-06-05 20:50 PDT, Wenson Hsieh	no flags	Details Formatted Diff Diff
Patch for upload (14.97 KB, patch) 2014-06-05 21:15 PDT, Wenson Hsieh	no flags	Details Formatted Diff Diff
Patch for landing (14.97 KB, patch) 2014-06-05 21:40 PDT, Wenson Hsieh	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Wenson Hsieh

Comment 1 2014-06-04 11:51:04 PDT

Created attachment 232489 [details] Patch

Wenson Hsieh

Comment 2 2014-06-04 11:58:06 PDT

(In reply to comment #0) > iOS8 media controls look different from iOS7 media controls <rdar://problem/16127943>

Eric Carlson

Comment 3 2014-06-04 14:43:25 PDT

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?

Wenson Hsieh

Comment 4 2014-06-05 15:24:57 PDT

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.

Wenson Hsieh

Comment 5 2014-06-05 15:43:21 PDT

Created attachment 232586 [details] Patch

Wenson Hsieh

Comment 6 2014-06-05 15:45:48 PDT

Please ignore the css comment at line 326 -- that should've been deleted. Sorry!

Wenson Hsieh

Comment 7 2014-06-05 16:10:24 PDT

Created attachment 232589 [details] Patch

Tim Horton

Comment 8 2014-06-05 19:30:39 PDT

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

Wenson Hsieh

Comment 9 2014-06-05 19:38:09 PDT

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.

Wenson Hsieh

Comment 10 2014-06-05 19:44:13 PDT

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!

Eric Carlson

Comment 11 2014-06-05 19:49:29 PDT

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.

Eric Carlson

Comment 12 2014-06-05 19:51:24 PDT

If you update the ChangeLog and upload the new patch without orphaning the old one, any committee can mark it cq+

Wenson Hsieh

Comment 13 2014-06-05 19:59:02 PDT

Created attachment 232599 [details] Patch

Wenson Hsieh

Comment 14 2014-06-05 20:11:18 PDT

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.

Wenson Hsieh

Comment 15 2014-06-05 20:13:32 PDT

Created attachment 232600 [details] Patch

Eric Carlson

Comment 16 2014-06-05 20:32:03 PDT

(In reply to comment #15) > Created an attachment (id=232600) [details] > Patch Did you upload the correct version of the patch?

Wenson Hsieh

Comment 17 2014-06-05 20:42:24 PDT

(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.

Wenson Hsieh

Comment 18 2014-06-05 20:50:40 PDT

Created attachment 232601 [details] Patch

Wenson Hsieh

Comment 19 2014-06-05 20:53:11 PDT

(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.

WebKit Commit Bot

Comment 20 2014-06-05 21:03:54 PDT

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

Wenson Hsieh

Comment 21 2014-06-05 21:15:14 PDT

Created attachment 232602 [details] Patch for upload

WebKit Commit Bot

Comment 22 2014-06-05 21:32:44 PDT

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

Wenson Hsieh

Comment 23 2014-06-05 21:40:42 PDT

Created attachment 232603 [details] Patch for landing

Jon Lee

Comment 24 2014-06-05 22:11:31 PDT

Comment on attachment 232603 [details] Patch for landing Clearing flags on attachment: 232603 Committed r169636: <http://trac.webkit.org/changeset/169636>

Note You need to log in before you can comment on or make changes to this bug.