Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2377193003:
Don't use absolute bounding boxes in LayoutVTTCue (Closed)
|
Created:
4 years, 2 months ago by fs Modified:
4 years, 2 months ago Reviewers:
foolip CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't use absolute bounding boxes in LayoutVTTCue
LayoutVTTCue was using absoluteContentBox()/absoluteBoundingBoxRect()
during overlap resolution. This would mean that boxes were computed
relative to the containing frame. The former also doesn't take
transforms into account, which would mean that the basic overlap check
against the title area would fail if a transform was present.
Instead compute the various bounding boxes relative to a common
ancestor, namely the text track container (which is also the containing
block of the cues.) Adjust the controls rect similarly to get it into the
same coordinate space.
BUG=647253
Committed: https://crrev.com/753ec4b0cfb34e187a485c9412ddeaca1604da3a
Cr-Commit-Position: refs/heads/master@{#422072}
Patch Set 1 #Patch Set 2 : Add test #
Total comments: 11
Patch Set 3 : Extend comments; adjust test #
Total comments: 1
Messages
Total messages: 24 (15 generated)
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
fs@opera.com changed reviewers: + foolip@chromium.org
Great comments, lgtm % nits https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html (right): https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html:5: transform: translate(1px, 0px); Haha, no more than necessary to test :) https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html:15: var video = document.currentScript.parentNode; AFAICT the test doesn't require the script to be inside the video element, right? Looks like some of the other tests here have it because there are multiple video elements, but for this I'd wonder if it's trying to tickle some corner case like https://bugs.chromium.org/p/chromium/issues/detail?id=350303 https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutVTTCue.cpp (right): https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutVTTCue.cpp:120: FloatQuad mappedContentQuad = box.localToAncestorQuad(cueContentBox, &ancestor, UseTransforms); Here or somewhere in this file it might be worth calling out why it's important that a local coordinate system is used. The test will fail if someone changes it, but without context I couldn't guess if "UseTransforms" here meant that transforms are applied like with getBoundingClientRect or ignored like with clientLeft and friends. https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutVTTCue.cpp:336: // should be a LayoutBox representing the part of the controls that are And I guess it can actually happen, like if you play with video::-webkit-media-text-track-*
https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html (right): https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html:15: var video = document.currentScript.parentNode; On 2016/09/29 at 19:25:19, foolip wrote: > AFAICT the test doesn't require the script to be inside the video element, right? Looks like some of the other tests here have it because there are multiple video elements, but for this I'd wonder if it's trying to tickle some corner case like https://bugs.chromium.org/p/chromium/issues/detail?id=350303 No, before 'load' should do I guess. I can "unwrap" the <video> (to <video></video><script>...) https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutVTTCue.cpp (right): https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutVTTCue.cpp:120: FloatQuad mappedContentQuad = box.localToAncestorQuad(cueContentBox, &ancestor, UseTransforms); On 2016/09/29 at 19:25:19, foolip wrote: > Here or somewhere in this file it might be worth calling out why it's important that a local coordinate system is used. The test will fail if someone changes it, but without context I couldn't guess if "UseTransforms" here meant that transforms are applied like with getBoundingClientRect or ignored like with clientLeft and friends. The reason we need UseTransforms here is because we use transforms (ha!) to layout non-line-snapped cues. Will add a comment. https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutVTTCue.cpp:336: // should be a LayoutBox representing the part of the controls that are On 2016/09/29 at 19:25:19, foolip wrote: > And I guess it can actually happen, like if you play with video::-webkit-media-text-track-* Right, "unlikely, but not impossible" (=> mostly). IIRC this is the layout box corresponding to -webkit-media-controls-panel ATM.
https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html (right): https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html:15: var video = document.currentScript.parentNode; On 2016/09/29 19:40:08, fs wrote: > On 2016/09/29 at 19:25:19, foolip wrote: > > AFAICT the test doesn't require the script to be inside the video element, > right? Looks like some of the other tests here have it because there are > multiple video elements, but for this I'd wonder if it's trying to tickle some > corner case like https://bugs.chromium.org/p/chromium/issues/detail?id=350303 > > No, before 'load' should do I guess. I can "unwrap" the <video> (to > <video></video><script>...) SGTM
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html (right): https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/track/track-cue-rendering-transformed-video.html:15: var video = document.currentScript.parentNode; On 2016/09/29 at 19:49:33, foolip wrote: > On 2016/09/29 19:40:08, fs wrote: > > On 2016/09/29 at 19:25:19, foolip wrote: > > > AFAICT the test doesn't require the script to be inside the video element, > > right? Looks like some of the other tests here have it because there are > > multiple video elements, but for this I'd wonder if it's trying to tickle some > > corner case like https://bugs.chromium.org/p/chromium/issues/detail?id=350303 > > > > No, before 'load' should do I guess. I can "unwrap" the <video> (to > > <video></video><script>...) > > SGTM Done. https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutVTTCue.cpp (right): https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutVTTCue.cpp:120: FloatQuad mappedContentQuad = box.localToAncestorQuad(cueContentBox, &ancestor, UseTransforms); On 2016/09/29 at 19:40:08, fs wrote: > On 2016/09/29 at 19:25:19, foolip wrote: > > Here or somewhere in this file it might be worth calling out why it's important that a local coordinate system is used. The test will fail if someone changes it, but without context I couldn't guess if "UseTransforms" here meant that transforms are applied like with getBoundingClientRect or ignored like with clientLeft and friends. > > The reason we need UseTransforms here is because we use transforms (ha!) to layout non-line-snapped cues. Will add a comment. Added. https://codereview.chromium.org/2377193003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutVTTCue.cpp:336: // should be a LayoutBox representing the part of the controls that are On 2016/09/29 at 19:40:08, fs wrote: > On 2016/09/29 at 19:25:19, foolip wrote: > > And I guess it can actually happen, like if you play with video::-webkit-media-text-track-* > > Right, "unlikely, but not impossible" (=> mostly). IIRC this is the layout box corresponding to -webkit-media-controls-panel ATM. Added additional note mentioning the potential pseudo element issue.
https://codereview.chromium.org/2377193003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutVTTCue.cpp (right): https://codereview.chromium.org/2377193003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutVTTCue.cpp:124: // We pass UseTransforms here primarily because we use a transform for Ah, this I did not realize, thanks for documenting!
The CQ bit was unchecked by fs@opera.com
The CQ bit was checked by fs@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org Link to the patchset: https://codereview.chromium.org/2377193003/#ps40001 (title: "Extend comments; adjust test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Don't use absolute bounding boxes in LayoutVTTCue LayoutVTTCue was using absoluteContentBox()/absoluteBoundingBoxRect() during overlap resolution. This would mean that boxes were computed relative to the containing frame. The former also doesn't take transforms into account, which would mean that the basic overlap check against the title area would fail if a transform was present. Instead compute the various bounding boxes relative to a common ancestor, namely the text track container (which is also the containing block of the cues.) Adjust the controls rect similarly to get it into the same coordinate space. BUG=647253 ========== to ========== Don't use absolute bounding boxes in LayoutVTTCue LayoutVTTCue was using absoluteContentBox()/absoluteBoundingBoxRect() during overlap resolution. This would mean that boxes were computed relative to the containing frame. The former also doesn't take transforms into account, which would mean that the basic overlap check against the title area would fail if a transform was present. Instead compute the various bounding boxes relative to a common ancestor, namely the text track container (which is also the containing block of the cues.) Adjust the controls rect similarly to get it into the same coordinate space. BUG=647253 Committed: https://crrev.com/753ec4b0cfb34e187a485c9412ddeaca1604da3a Cr-Commit-Position: refs/heads/master@{#422072} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/753ec4b0cfb34e187a485c9412ddeaca1604da3a Cr-Commit-Position: refs/heads/master@{#422072} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
