Bug 130608

Summary: [GTK] Volume slider shows below the panel with videos in certain cases
Product: WebKit Reporter: Xabier Rodríguez Calvar <calvaris>
Component: New BugsAssignee: Xabier Rodríguez Calvar <calvaris>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, eric.carlson, glenn, jer.noble, mrobinson, philipj, rniwa, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
none
Patch none

Xabier Rodríguez Calvar
Reported 2014-03-21 10:56:06 PDT
[GTK] Volume slider shows below the panel with videos in certain cases
Attachments
Patch (6.28 KB, patch)
2014-03-21 10:57 PDT, Xabier Rodríguez Calvar
no flags
Patch (6.47 KB, patch)
2014-03-21 11:15 PDT, Xabier Rodríguez Calvar
no flags
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (474.09 KB, application/zip)
2014-03-21 13:19 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (507.04 KB, application/zip)
2014-03-21 13:30 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (545.36 KB, application/zip)
2014-03-21 13:47 PDT, Build Bot
no flags
Patch (7.93 KB, patch)
2014-03-22 03:49 PDT, Xabier Rodríguez Calvar
no flags
Xabier Rodríguez Calvar
Comment 1 2014-03-21 10:57:54 PDT
Martin Robinson
Comment 2 2014-03-21 11:03:30 PDT
Comment on attachment 227458 [details] Patch Can you explain why this was failing before?
Xabier Rodríguez Calvar
Comment 3 2014-03-21 11:15:45 PDT
Xabier Rodríguez Calvar
Comment 4 2014-03-21 11:16:36 PDT
(In reply to comment #2) > (From update of attachment 227458 [details]) > Can you explain why this was failing before? Sure, I forgot to explain it in the changelog, so reuploaded the patch.
Martin Robinson
Comment 5 2014-03-21 11:19:07 PDT
Comment on attachment 227463 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=227463&action=review Looks good. A couple small nits for landing: > Source/WebCore/ChangeLog:9 > + We need to delay the moment we check if the volume slider show > + show up or down because if the video was not visible when we were Double "show" > LayoutTests/media/video-initially-hidden-volume-slider-up-expected.txt:5 > +** Test that the volume slider is rendered correctly if video is brought to foreground ** > +** Move mouse on top of the mute button ** > +** Ensure layout is done after mouse move ** > +** The volume slider should not be positioned below the panel ** I think these can just be code comments.
Build Bot
Comment 6 2014-03-21 13:19:33 PDT
Comment on attachment 227463 [details] Patch Attachment 227463 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6752269651410944 New failing tests: media/video-initially-hidden-volume-slider-up.html
Build Bot
Comment 7 2014-03-21 13:19:37 PDT
Created attachment 227479 [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.5
Build Bot
Comment 8 2014-03-21 13:30:04 PDT
Comment on attachment 227463 [details] Patch Attachment 227463 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4879779337601024 New failing tests: media/video-initially-hidden-volume-slider-up.html
Build Bot
Comment 9 2014-03-21 13:30:09 PDT
Created attachment 227482 [details] Archive of layout-test-results from webkit-ews-07 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Build Bot
Comment 10 2014-03-21 13:46:56 PDT
Comment on attachment 227463 [details] Patch Attachment 227463 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5351491837624320 New failing tests: media/video-initially-hidden-volume-slider-up.html
Build Bot
Comment 11 2014-03-21 13:47:02 PDT
Created attachment 227486 [details] Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Xabier Rodríguez Calvar
Comment 12 2014-03-22 03:49:10 PDT
Xabier Rodríguez Calvar
Comment 13 2014-03-22 06:38:13 PDT
(In reply to comment #5) > > Source/WebCore/ChangeLog:9 > > + We need to delay the moment we check if the volume slider show > > + show up or down because if the video was not visible when we were > > Double "show" I rephrased it. > > LayoutTests/media/video-initially-hidden-volume-slider-up-expected.txt:5 > > +** Test that the volume slider is rendered correctly if video is brought to foreground ** > > +** Move mouse on top of the mute button ** > > +** Ensure layout is done after mouse move ** > > +** The volume slider should not be positioned below the panel ** > > I think these can just be code comments. Done. I also fixed the problem with the test in Mac.
Xabier Rodríguez Calvar
Comment 14 2014-03-24 16:20:42 PDT
(In reply to comment #13) > (In reply to comment #5) > > > Source/WebCore/ChangeLog:9 > > > + We need to delay the moment we check if the volume slider show > > > + show up or down because if the video was not visible when we were > > > > Double "show" > > I rephrased it. > > > > LayoutTests/media/video-initially-hidden-volume-slider-up-expected.txt:5 > > > +** Test that the volume slider is rendered correctly if video is brought to foreground ** > > > +** Move mouse on top of the mute button ** > > > +** Ensure layout is done after mouse move ** > > > +** The volume slider should not be positioned below the panel ** > > > > I think these can just be code comments. > > Done. > > I also fixed the problem with the test in Mac. Apart from this changes, the fix for Mac 'required' to change some common Mac code, I think a review for that would be necessary as I think the other r+ is not enough for this.
Jer Noble
Comment 15 2014-03-24 17:00:17 PDT
Comment on attachment 227546 [details] Patch r=me, based on the Mac changes & Martin's earlier r+ and
Jer Noble
Comment 16 2014-03-24 17:00:40 PDT
(In reply to comment #15) > (From update of attachment 227546 [details]) > r=me, based on the Mac changes & Martin's earlier r+ and s/ and/.
WebKit Commit Bot
Comment 17 2014-03-25 02:10:59 PDT
Comment on attachment 227546 [details] Patch Clearing flags on attachment: 227546 Committed r166227: <http://trac.webkit.org/changeset/166227>
WebKit Commit Bot
Comment 18 2014-03-25 02:11:05 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.