[webkit-reviews] review denied: [Bug 118245] Wrong linebox height, when block element parent has vertical-align property defined. : [Attachment 205885] Patch

Wed Jul 3 09:55:17 PDT 2013

Dave Hyatt <hyatt at apple.com> has denied Zalan Bujtas <zalan at apple.com>'s
request for review:
Bug 118245: Wrong linebox height, when block element parent has vertical-align
property defined.
https://bugs.webkit.org/show_bug.cgi?id=118245

Attachment 205885: Patch
https://bugs.webkit.org/attachment.cgi?id=205885&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=205885&action=review

r-

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2511
> +    if ((parent->isInline() && flow->style()->verticalAlign() !=
parent->style()->verticalAlign())
> +	   || (!parent->isInline() && flow->style()->verticalAlign() !=
BASELINE))
>	   return true;

Is this really a block issue? I'm suspicious.

<!DOCTYPE html>
 <html>
 <head>
   <style>
    .va { 
       width: 30px; 
      background: green; 
      vertical-align: 100px;
     }
   </style>
 </head>
 <body>
  <div>This tests if the vertical-align on the block element is ignored, when
the child inline element is empty</div>
  <div><span class="va"><span></span></span><input type="submit"></div>
 </body>
 </html>

The above also looks like a regression to me from shipping Safari, and it
involves only spans.

Looking into the code some more, it looks like:

&& !alwaysRequiresLineBox(toRenderInline(it.m_obj), lineInfo) &&
!requiresLineBoxForContent(toRenderInline(it.m_obj), lineInfo)

might be the problem. If you're an empty inline, then you return false from
alwaysRequiresLineBox, and so that condition of the and is now true. Maybe I'm
missing something, but I would have thought an empty inline should never apply
line-height or vertical-alignment. It sure seems like the above two functions
should simply merge, with the empty-without-borders-padding condition just
causing requiresLineBoxForContent to return false.

It does look like before the patch we were allowing line-height to apply to
empty inlines and trump them, but I'm not sure that was correct. Maybe Robert
can show a counter-example. I don't think empty inlines should ever get line
boxes (unless they have border/padding that make them non-empty of course).

Anyway, I don't think that's the issue. I think we just want to treat the
vertical-align values of "top" and "bottom" as though they are not equivalent,
since I think you could cause the same bug to occur with nested inlines.