Bug 133521 - iOS8 media controls look different from iOS7 media controls
Summary: iOS8 media controls look different from iOS7 media controls
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-06-04 11:46 PDT by Wenson Hsieh
Modified: 2014-06-05 22:12 PDT (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2014-06-04 11:46:20 PDT
iOS8 media controls look different from iOS7 media controls
Comment 1 Wenson Hsieh 2014-06-04 11:51:04 PDT
Created attachment 232489 [details]
Patch
Comment 2 Wenson Hsieh 2014-06-04 11:58:06 PDT
(In reply to comment #0)
> iOS8 media controls look different from iOS7 media controls

<rdar://problem/16127943>
Comment 3 Eric Carlson 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?
Comment 4 Wenson Hsieh 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.
Comment 5 Wenson Hsieh 2014-06-05 15:43:21 PDT
Created attachment 232586 [details]
Patch
Comment 6 Wenson Hsieh 2014-06-05 15:45:48 PDT
Please ignore the css comment at line 326 -- that should've been deleted. Sorry!
Comment 7 Wenson Hsieh 2014-06-05 16:10:24 PDT
Created attachment 232589 [details]
Patch
Comment 8 Tim Horton 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
Comment 9 Wenson Hsieh 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.
Comment 10 Wenson Hsieh 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!
Comment 11 Eric Carlson 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.
Comment 12 Eric Carlson 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+
Comment 13 Wenson Hsieh 2014-06-05 19:59:02 PDT
Created attachment 232599 [details]
Patch
Comment 14 Wenson Hsieh 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.
Comment 15 Wenson Hsieh 2014-06-05 20:13:32 PDT
Created attachment 232600 [details]
Patch
Comment 16 Eric Carlson 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?
Comment 17 Wenson Hsieh 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.
Comment 18 Wenson Hsieh 2014-06-05 20:50:40 PDT
Created attachment 232601 [details]
Patch
Comment 19 Wenson Hsieh 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.
Comment 20 WebKit Commit Bot 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
Comment 21 Wenson Hsieh 2014-06-05 21:15:14 PDT
Created attachment 232602 [details]
Patch for upload
Comment 22 WebKit Commit Bot 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
Comment 23 Wenson Hsieh 2014-06-05 21:40:42 PDT
Created attachment 232603 [details]
Patch for landing
Comment 24 Jon Lee 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>