Don't use absolute bounding boxes in LayoutVTTCue

third_party/WebKit/Source/core/layout/LayoutVTTCue.cpp

 /*
  * Copyright (C) 2012 Victor Carbune (victor@rosedu.org)
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 15 matching lines @@
 #include "core/layout/LayoutVTTCue.h"
 
 #include "core/frame/Settings.h"
 #include "core/html/shadow/MediaControls.h"
 #include "core/layout/LayoutInline.h"
 #include "core/layout/LayoutState.h"
 #include "wtf/MathExtras.h"
 
 namespace blink {
 
-LayoutVTTCue::LayoutVTTCue(ContainerNode* node, float snapToLinesPosition)
-    : LayoutBlockFlow(node)
-    , m_snapToLinesPosition(snapToLinesPosition)
-{
-}
+namespace {
 
 class SnapToLinesLayouter {
     STACK_ALLOCATED();
 public:
     SnapToLinesLayouter(LayoutVTTCue& cueBox, const IntRect& controlsRect)
         : m_cueBox(cueBox)
         , m_controlsRect(controlsRect)
         , m_margin(0.0)
     {
         if (Settings* settings = m_cueBox.document().settings())
@@ skipping 24 matching lines @@
 InlineFlowBox* SnapToLinesLayouter::findFirstLineBox() const
 {
     if (!m_cueBox.firstChild()->isLayoutInline())
         return nullptr;
     return toLayoutInline(m_cueBox.firstChild())->firstLineBox();
 }
 
 LayoutUnit SnapToLinesLayouter::computeInitialPositionAdjustment(LayoutUnit& step, LayoutUnit maxDimension,
     LayoutUnit margin) const
 {
-    ASSERT(std::isfinite(m_cueBox.snapToLinesPosition()));
+    DCHECK(std::isfinite(m_cueBox.snapToLinesPosition()));
 
     // 6. Let line be cue's computed line.
     // 7. Round line to an integer by adding 0.5 and then flooring it.
     LayoutUnit linePosition(floorf(m_cueBox.snapToLinesPosition() + 0.5f));
 
     WritingMode writingMode = m_cueBox.style()->getWritingMode();
     // 8. Vertical Growing Left: Add one to line then negate it.
     if (writingMode == RightToLeftWritingMode)
         linePosition = -(linePosition + 1);
 
@@ skipping 14 matching lines @@
 
         // ... and negate step.
         step = -step;
     } else {
         // ... Otherwise, increase position by margin.
         position += margin;
     }
     return position;
 }
 
+IntRect contentBoxRelativeToAncestor(
+    const LayoutBox& box, const LayoutBoxModelObject& ancestor)
+{
+    FloatRect cueContentBox(box.contentBoxRect());
+    FloatQuad mappedContentQuad = box.localToAncestorQuad(cueContentBox, &ancestor, UseTransforms);

[foolip, 2016/09/29 19:25:19]

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.

[fs, 2016/09/29 19:40:08]

The reason we need UseTransforms here is because we use transforms (ha!) to
layout non-line-snapped cues. Will add a comment.

[fs, 2016/09/30 08:05:32]

Added.

+    return mappedContentQuad.enclosingBoundingBox();
+}
+
+IntRect cueBoundingBox(const LayoutBox& cueBox)
+{
+    return contentBoxRelativeToAncestor(cueBox, *cueBox.containingBlock());
+}
+
 bool SnapToLinesLayouter::isOutside(const IntRect& titleArea) const
 {
-    return !titleArea.contains(m_cueBox.absoluteContentBox());
+    return !titleArea.contains(cueBoundingBox(m_cueBox));
 }
 
 bool SnapToLinesLayouter::isOverlapping() const
 {
-    IntRect cueBoxRect = m_cueBox.absoluteBoundingBoxRect();
-    for (LayoutObject* box = m_cueBox.previousSibling(); box; box = box->previousSibling()) {
-        IntRect boxRect = box->absoluteBoundingBoxRect();
-
-        if (cueBoxRect.intersects(boxRect))
+    IntRect cueBoxRect = cueBoundingBox(m_cueBox);
+    for (LayoutBox* box = m_cueBox.previousSiblingBox(); box; box = box->previousSiblingBox()) {
+        if (cueBoxRect.intersects(cueBoundingBox(*box)))
             return true;
     }
-
-    if (cueBoxRect.intersects(m_controlsRect))
-        return true;
-
-    return false;
+    return cueBoxRect.intersects(m_controlsRect);
 }
 
 bool SnapToLinesLayouter::shouldSwitchDirection(InlineFlowBox* firstLineBox, LayoutUnit step, LayoutUnit margin) const
 {
     // 17. Horizontal: If step is negative and the top of the first line box in
     // boxes is now above the top of the title area, or if step is positive and
     // the bottom of the first line box in boxes is now below the bottom of the
     // title area, jump to the step labeled switch direction.
     // Vertical: If step is negative and the left edge of the first line
     // box in boxes is now to the left of the left edge of the title area, or
@@ skipping 55 matching lines @@
 
     // XX. Let switched be false.
     bool switched = false;
 
     // 14. Horizontal: Let title area be a box that covers all of the video's
     // rendering area except for a height of margin at the top of the rendering
     // area and a height of margin at the bottom of the rendering area.
     // Vertical: Let title area be a box that covers all of the video’s
     // rendering area except for a width of margin at the left of the rendering
     // area and a width of margin at the right of the rendering area.
-    IntRect titleArea = m_cueBox.containingBlock()->absoluteBoundingBoxRect();
+    IntRect titleArea = enclosingIntRect(m_cueBox.containingBlock()->contentBoxRect());
     if (blink::isHorizontalWritingMode(writingMode)) {
         titleArea.move(0, margin.toInt());
         titleArea.contract(0, (2 * margin).toInt());
     } else {
         titleArea.move(margin.toInt(), 0);
         titleArea.contract((2 * margin).toInt(), 0);
     }
 
     // 15. Step loop: If none of the boxes in boxes would overlap any of the
     // boxes in output, and all of the boxes in output are entirely within the
@@ skipping 32 matching lines @@
         // 22. Negate step.
         step = -step;
 
         // 23. Set switched to true.
         switched = true;
 
         // 24. Jump back to the step labeled step loop.
     }
 }
 
+} // unnamed namespace
+
+LayoutVTTCue::LayoutVTTCue(ContainerNode* node, float snapToLinesPosition)
+    : LayoutBlockFlow(node)
+    , m_snapToLinesPosition(snapToLinesPosition)
+{
+}
+
 void LayoutVTTCue::repositionCueSnapToLinesNotSet()
 {
     // FIXME: Implement overlapping detection when snap-to-lines is not set. http://wkb.ug/84296
 
     // http://dev.w3.org/html5/webvtt/#dfn-apply-webvtt-cue-settings
     // Step 13, "If cue's text track cue snap-to-lines flag is not set".
 
     // 1. Let bounding box be the bounding box of the boxes in boxes.
 
     // 2. Run the appropriate steps from the following list:
@@ skipping 24 matching lines @@
     // position to their current position, and then jump to the step labeled
     // done positioning below. If there are multiple such positions that are
     // equidistant from their current position, use the highest one amongst
     // them; if there are several at that height, then use the leftmost one
     // amongst them.
 
     // 5. Otherwise, jump to the step labeled done positioning below. (The
     // boxes will unfortunately overlap.)
 }
 
+IntRect LayoutVTTCue::computeControlsRect() const
+{
+    // Determine the area covered by the media controls, if any. If the controls
+    // are present, they are the next sibling of the text track container, which
+    // is our parent. (LayoutMedia ensures that the media controls are laid out
+    // before text tracks, so that the layout is up to date here.)
+    DCHECK(parent()->node()->isTextTrackContainer());
+    LayoutObject* controlsContainer = parent()->nextSibling();
+    if (!controlsContainer)
+        return IntRect();
+    // Only a part of the media controls is used for overlap avoidance.
+    MediaControls* controls = toMediaControls(controlsContainer->node());
+    LayoutObject* controlsLayout = controls->layoutObjectForTextTrackLayout();
+    // The (second part of) following is mostly defensive - in general there
+    // should be a LayoutBox representing the part of the controls that are

[foolip, 2016/09/29 19:25:19]

And I guess it can actually happen, like if you play with
video::-webkit-media-text-track-*

[fs, 2016/09/29 19:40:08]

Right, "unlikely, but not impossible" (=> mostly). IIRC this is the layout box
corresponding to -webkit-media-controls-panel ATM.

[fs, 2016/09/30 08:05:32]

Added additional note mentioning the potential pseudo element issue.

+    // relevant for overlap avoidance.
+    if (!controlsLayout || !controlsLayout->isBox())
+        return IntRect();
+    // Assume that the controls container are positioned in the same relative
+    // position as the text track container. (LayoutMedia::layout ensures this.)
+    return contentBoxRelativeToAncestor(
+        toLayoutBox(*controlsLayout), toLayoutBox(*controlsContainer));
+}
+
 void LayoutVTTCue::layout()
 {
     LayoutBlockFlow::layout();
 
-    ASSERT(firstChild());
+    DCHECK(firstChild());
 
     LayoutState state(*this, locationOffset());
 
-    // Determine the area covered by the media controls, if any. If the controls
-    // are present, they are the next sibling of the text track container, which
-    // is our parent. (LayoutMedia ensures that the media controls are laid out
-    // before text tracks, so that the layout is up to date here.)
-    ASSERT(parent()->node()->isTextTrackContainer());
-    IntRect controlsRect;
-    if (LayoutObject* parentSibling = parent()->nextSibling()) {
-        // Only a part of the media controls is used for overlap avoidance.
-        MediaControls* controls = toMediaControls(parentSibling->node());
-        if (LayoutObject* controlsLayout = controls->layoutObjectForTextTrackLayout())
-            controlsRect = controlsLayout->absoluteBoundingBoxRect();
-    }
-
     // http://dev.w3.org/html5/webvtt/#dfn-apply-webvtt-cue-settings - step 13.
     if (!std::isnan(m_snapToLinesPosition))
-        SnapToLinesLayouter(*this, controlsRect).layout();
+        SnapToLinesLayouter(*this, computeControlsRect()).layout();
     else
         repositionCueSnapToLinesNotSet();
 }
 
 } // namespace blink