Closed Bug 1330962 Opened 6 years ago Closed 6 years ago

inline-block with a display:block <input> child element has a wrong baseline

Categories

(Core :: Layout: Form Controls, defect, P3)

Product:
Core
Component:
Layout: Form Controls
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: jsnajdr, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: testcase)

Attachments

(6 files, 3 obsolete files)

Testcase
3.69 KB, text/html
Details
part 4 - Cleanup some #includes and make nsLegendFrame 'final'.
5.46 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
part 5 - Reftests for baseline alignment of various form controls.
18.78 KB, patch
Details | Diff | Splinter Review
part 1 - Add baseline methods to nsTextControlFrame.
5.55 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
part 2 - Add baseline methods to nsHTMLButtonControlFrame.
3.61 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
part 3 - Add baseline methods to nsFieldSetFrame.
4.70 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review 
Steps to reproduce:

This is a UI to register a subdomain for a service:

<body>
  <div style="display:inline-block">
    <label>Team URL</label>
    <input style="display:block;text-align:right" type="text" value="company"></input>
  </div>
  <span>.domain.com</span>
</body>

Expected result:
The baselines of the input and the span are aligned on a common baseline, and together form a "company.domain.com" domain name.

Actual result:
The baselines of the label (Team URL) and the ".domain.com" span are aligned, the UI is broken.

Used browser: latest Nightly (FF 53)

Chrome and Safari show the expected result, Firefox (and Servo too, by the way) shows the wrong result.

I'm not sure what the spec says about this, so it's entirely possible that Firefox behavior is correct and there is a bug in Chrome/Safari. But it's definitely a webcompat issue. I found this issue on a real website (productboard.com). As often happens, developed and tested on Chrome, broken in Firefox.
Attached file Testcase — 
Looks like Chrome pick up a baseline for a few other controls as well...
Has STR: --- → yes
Component: Layout: Block and Inline → Layout: Form Controls
Keywords: testcase
OS: Unspecified → All
Priority: -- → P3
Hardware: Unspecified → All
Summary: Box with an input element child has a wrong baseline → inline-block with a display:block <input> child element has a wrong baseline
Assignee: nobody → mats

        Attachment #8829035 -
        Flags: review?(dholbert)

    
    

    
  


  

        Attachment #8829036 -
        Flags: review?(dholbert)

    
    

    
  


  

        Attachment #8829038 -
        Flags: review?(dholbert)

    
    

    
  
These patches makes us compatible with Chrome for the testcase in the first attachment.
Edge is mostly compatible, but it seems they additionally implement baselines for block
<textarea>s, but they don't implement baselines for block scroll frames in other cases.

    
    

    
  
Comment on attachment 8829038 [details] [diff] [review]
part 3 - Add baseline methods to nsFieldSetFrame.

> +    case mozilla::StyleDisplay::Grid:

FYI, the reason I'm using the full name here even though we have a "using namespace mozilla" at the top of this file is that GCC fails to compile it without the "mozilla::" prefix. :-(

        Attachment #8829039 -
        Flags: review?(dholbert) → review+

    
    

    
  
Comment on attachment 8829035 [details] [diff] [review]
part 1 - Add baseline methods to nsTextControlFrame.

Review of attachment 8829035 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/forms/nsTextControlFrame.cpp
@@ +104,5 @@
>  #endif
>  
>  nsTextControlFrame::nsTextControlFrame(nsStyleContext* aContext)
>    : nsContainerFrame(aContext)
> +  , mFirstBaseline(ReflowOutput::ASK_FOR_BASELINE)

I'm not sure ASK_FOR_BASELINE is the right meaningful sentinel-value to use here.

Right now, it looks like that enum-value *only* has meaning inside of ReflowOutput (that's the only place we set it / check for it, I think).  And there, it means "Ask the frame for more details" -- so it's a bit odd that the *frame itself* might also be storing the same sentinel value as its own answer that it might provide. (Odd for the frame to say "ask the frame".)

In nsFlexContainerFrame and nsGridContainerFrame, we initialize the equivalent members to NS_INTRINSIC_WIDTH_UNKNOWN. Maybe we should use that same sentinel here?

@@ +547,5 @@
>      nsLayoutUtils::GetCenteredFontBaseline(fontMet, lineHeight,
>                                             wm.IsLineInverted()) +
> +    aReflowInput.ComputedLogicalBorderPadding().BStart(wm);
> +  if (MOZ_UNLIKELY(mFirstBaseline == ReflowOutput::ASK_FOR_BASELINE)) {
> +    mFirstBaseline = ReflowOutput::ASK_FOR_BASELINE - 1;

What's going on with this magic "ASK_FOR_BASELINE - 1" value?

Perhaps this is just to avoid tripping the MOZ_ASSERT that I noted in the header file, when the author uses bogusly huge sizes that happen to produce this as an actual baseline value?  If so: I expect you can get rid of this special case if you adjust the assert as I suggested.  (Or if there's some other reason for this, it definitely merits a code comment to explain what's going on...)

::: layout/forms/nsTextControlFrame.h
@@ +75,5 @@
> +    if (!IsSingleLineTextControl()) {
> +      return false;
> +    }
> +    MOZ_ASSERT(mFirstBaseline != ReflowOutput::ASK_FOR_BASELINE,
> +               "please call Reflow before asking for the baseline");

Fuzzer testcases can obviously trip this with huge sizes.  Let's make this "have we reflowed" check more explicit & unfuzzable, like so:
  MOZ_ASSERT(!HasAnyStateBits(NS_FRAME_FIRST_REFLOW),
             "please call Reflow [etc]");

        Attachment #8829035 -
        Flags: review?(dholbert) → review-

    
    

    
  
Comment on attachment 8829036 [details] [diff] [review]
part 2 - Add baseline methods to nsHTMLButtonControlFrame.

Review of attachment 8829036 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/forms/nsHTMLButtonControlFrame.cpp
@@ +361,5 @@
> +                                         nscoord* aBaseline) const
> +{
> +  nsIFrame* inner = mFrames.FirstChild();
> +  if (MOZ_UNLIKELY(inner->GetWritingMode().IsOrthogonalTo(aWM) ||
> +                   !inner->GetNaturalBaselineBOffset(aWM, aBaselineGroup, aBaseline))) {

This line is too long -- wrap after "aBaselineGroup," perhaps.

@@ +366,5 @@
> +    return false;
> +  }
> +  if (MOZ_UNLIKELY(inner->GetWritingMode().IsOrthogonalTo(aWM))) {
> +    return false;
> +  }

This second early-return is redundant. It's already handled as the first early return here (the first condition of that if-check).

So, unless I'm missing something, please drop these last 3 lines.

@@ +369,5 @@
> +    return false;
> +  }
> +  if (!inner->GetNaturalBaselineBOffset(aWM, aBaselineGroup, aBaseline)) {
> +    // <input type=color> has an empty block frame as inner frame
> +    *aBaseline = inner->

This is redundant too... This condition is also covered in the first early-return (though with different behavior!).

So, this if-check will never be entered right now, IIUC...

r- for now, since it looks like something's wrong here.

        Attachment #8829036 -
        Flags: review?(dholbert) → review-

    
    

    
  
Comment on attachment 8829038 [details] [diff] [review]
part 3 - Add baseline methods to nsFieldSetFrame.

Review of attachment 8829038 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with GetParent() calls removed (or explained) as noted below

::: layout/forms/nsFieldSetFrame.cpp
@@ +651,5 @@
> +  MOZ_ASSERT(!inner->GetWritingMode().IsOrthogonalTo(aWM));
> +  if (!inner->GetVerticalAlignBaseline(aWM, aBaseline)) {
> +    return false;
> +  }
> +  nscoord innerBStart = inner->BStart(aWM, GetParent()->GetSize());

I'm pretty sure you really want to drop the GetParent() here.  (so just pass in "GetSize()", not "GetParent()->GetSize()".)

The inner frame's position is represented (in its mRect) with respect to *our* origin, AFAICT -- it's an offset from the nsFieldSetFrame's origin, not the nsFieldSetFrame's parent's origin.  So that means we should be passing the nsFieldSetFrame's size into this inner->BStart method, so that it can subtract from that size in the unlikely event that it needs to flip the representation to correct for writing-mode antiparallelism.

@@ +666,5 @@
> +  MOZ_ASSERT(!inner->GetWritingMode().IsOrthogonalTo(aWM));
> +  if (!inner->GetNaturalBaselineBOffset(aWM, aBaselineGroup, aBaseline)) {
> +    return false;
> +  }
> +  nscoord innerBStart = inner->BStart(aWM, GetParent()->GetSize());

Same here -- I don't think you want the GetParent() call here.

        Attachment #8829038 -
        Flags: review?(dholbert) → review+

    
    

    
  
(In reply to Daniel Holbert [:dholbert] from comment #10)
> Fuzzer testcases can obviously trip this with huge sizes.  Let's make this
> "have we reflowed" check more explicit & unfuzzable, like so:
>   MOZ_ASSERT(!HasAnyStateBits(NS_FRAME_FIRST_REFLOW),
>              "please call Reflow [etc]");

Unfortunately, NS_FRAME_FIRST_REFLOW is only set once and never again,
so this has limited value.  Using a sentinel value is better, especially
if we reset it in MarkIntrinsicISizesDirty(), which I forgot in the first
version.  Yes, this can give false positives with nscoord overflows, so
I've made it non-fatal.

    
    

    
  


  

        Attachment #8829035 -
        Attachment is obsolete: true

        Attachment #8830502 -
        Flags: review?(dholbert)

    
    

    
  


  
> This second early-return is redundant.

Good catch, but it was actually the first chunk that I forgot to remove.
(I'll add some tests that would have caught that eventually, but that
needs some baseline call tweaks in Grid or Flexbox first I think.)

        Attachment #8829036 -
        Attachment is obsolete: true

        Attachment #8830503 -
        Flags: review?(dholbert)

    
    

    
  


  
> I'm pretty sure you really want to drop the GetParent() here.

Right, I actually saw that too, but figured I should just keep it
as is for now before I had a chance to investigate it.
nsFieldSetFrame layout is pretty weird after all. :-)

But since you agree, let's just fix that and hope we're right.

        Attachment #8829038 -
        Attachment is obsolete: true

        Attachment #8830505 -
        Flags: review+

        Attachment #8830502 -
        Flags: review?(dholbert) → review+

        Attachment #8830503 -
        Flags: review?(dholbert) → review+

    
    

    
  
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc7508381dc1
part 1 - Add baseline methods to nsTextControlFrame.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b92b30ed1d3
part 2 - Add baseline methods to nsHTMLButtonControlFrame.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/2394701998b2
part 3 - Add baseline methods to nsFieldSetFrame.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cfebee598a6
part 4 - Cleanup some #includes and make nsLegendFrame 'final'.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/71c277105903
part 5 - Reftests for baseline alignment of various form controls.

    
    

    
  
> for fairly comprehensible Windows no-accel failures ...

Some anti-aliasing differences that I missed because they didn't show up in my
Try run (comment 6) because "-p all" apparently doesn't include Win8.

> ... and incomprehensible Android debug failures

Yeah, that failure is unrelated to the code changes, but the failure comes from
adding the new reftests.  Here's a Try run with only part 5 (the tests) that
also fails in layout/reftests/bugs/315920-17.html :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=512b79c26c636f20f4d7675286df8c4943086337&exclusion_profile=false&selectedJob=72824028
I'm guessing adding the new tests changes the reftest chunks somehow.

    
    

    
  
layout/reftests/bugs/315920-17.html is the very first test in R15 in the Try run above.

    
    

    
  
I don't see any obvious focus dependencies in that test, but I added a "needs-focus"
anyway to see if it helps:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9922b3dfd5b0b8c1641b8a94418f84f63a76362e

    
    

    
  
Nope, same failure.  OK, let's try "MozReftestInvalidate" instead of "onload" then:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=be592c00cbc062c295bee9a612c141a2a62f9ed8

    
    

    
  
(In reply to Mats Palmgren (:mats) from comment #19)
> "-p all" apparently doesn't include Win8.

-p all does, it's -u reftest that doesn't. I think the least typing which currently does include Win8, without excluding anything else and without running things known to be broken for trunk pushes, is something like -u reftest[-10.6,-Windows XP]. - to negate a filter (when it is the first filter) has the magic property of including all non-default platforms, so before we started breaking platforms on the trunk it was -u reftest[-goats], but now you don't want to have to see the burning 10.6 and WinXP jobs that would produce.

    
    

    
  
Thanks for the tip!  I'll use that henceforth when I want "all" reftests.

    
    

    
  
The "MozReftestInvalidate" fix solved the layout/reftests/bugs/315920-17.html failure.
Hmm, it's kind of odd that it only needs that when it's the first test in the chunk.

Now let's see if switching to an Ahem font helps the anti-aliasing issue on Windows:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=62a6a67a0c481b7cccf1defc749f48abeec54b22

    
    

    
  
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5318543f3e1
part 1 - Add baseline methods to nsTextControlFrame.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/095a98547a79
part 2 - Add baseline methods to nsHTMLButtonControlFrame.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/24b45bba9d5c
part 3 - Add baseline methods to nsFieldSetFrame.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1588f6e50d2
part 4 - Cleanup some #includes and make nsLegendFrame 'final'.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0acc072ee76
part 5 - Reftests for baseline alignment of various form controls.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b222ec9a5d90
part 6 - Start this test using MozReftestInvalidate rather than onload.
Depends on: 1383938

          You need to log in
          before you can comment on or make changes to this bug.