Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(615)

Issue 1166743004: Force display:none for audio elements with no controls attribute (Closed)

Created:
5 years, 6 months ago by philipj_slow
Modified:
5 years, 6 months ago
Reviewers:
rune
CC:
blink-reviews, nessy, mlamouri+watch-blink_chromium.org, blink-reviews-css, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, apavlov+blink_chromium.org, darktears, vcarbune.chromium, rwlbuis
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Force display:none for audio elements with no controls attribute In label-contains-other-interactive-content.html there is an audio element which is clicked twice, first without the controls attribute and then again after adding it. When there is no controls attribute the element doesn't have a layout object, so that amounted to clicking at (0,0). Due to the bug addressed here, adding the controls attribute also didn't cause the layout object to be created when it should, so the second click was also at (0,0). These two clicks add up to a double click, triggering a selection in the page. This is the cause of the test failure in a seemingly unrelated change: https://codereview.chromium.org/838003005/ Change label-contains-other-interactive-content.html to not (fail to) click on the audio element with no controls attribute, as it's far from obvious that clickElement(element) will click (0,0) instead. BUG=497120 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196671

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+29 lines, -1 line) Patch
M LayoutTests/fast/forms/label/label-contains-other-interactive-content.html View 1 chunk +1 line, -1 line 0 comments Download
A LayoutTests/media/audio-controls-computed-display.html View 1 chunk +24 lines, -0 lines 0 comments Download
M Source/core/css/mediaControls.css View 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 11 (4 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1166743004/1
5 years, 6 months ago (2015-06-08 10:39:02 UTC) #2
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 6 months ago (2015-06-08 11:29:40 UTC) #4
philipj_slow
PTAL, instead of https://codereview.chromium.org/838003005/
5 years, 6 months ago (2015-06-08 11:33:00 UTC) #6
philipj_slow
On 2015/06/08 11:33:00, philipj wrote: > PTAL, instead of https://codereview.chromium.org/838003005/ Sorry, I meant https://codereview.chromium.org/1170583002/
5 years, 6 months ago (2015-06-08 11:33:17 UTC) #7
rune
lgtm
5 years, 6 months ago (2015-06-08 12:32:34 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1166743004/1
5 years, 6 months ago (2015-06-08 12:40:21 UTC) #10
commit-bot: I haz the power
5 years, 6 months ago (2015-06-08 12:44:12 UTC) #11
Message was sent while issue was closed.
Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196671

Powered by Google App Engine
This is Rietveld 408576698