File input draws way outside its bounds

NEW
Unassigned

Status

()

P3
normal
3 years ago
2 years ago

People

(Reporter: mstange, Unassigned)

Tracking

({polish})

Trunk
polish
Points:
---

Firefox Tracking Flags

(firefox48 affected)

Details

(Whiteboard: tpi:+)

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
Created attachment 8736563 [details]
reftest failure log

I'm hitting a reftest failure in this file:

https://dxr.mozilla.org/mozilla-central/source/layout/reftests/flexbox/flexbox-widget-flex-items-3.html
https://hg.mozilla.org/mozilla-central/raw-file/494289c72ba3997183e7b5beaca3e0447ecaf96d/layout/reftests/flexbox/flexbox-widget-flex-items-3.html

My patch is changing how we simplify the invalidation region, and the reftest failure shows differing amounts of the <input type="file"> button painted outside the dashed border. I think this is not a bug in my invalidation patch, but instead a bug in nsNativeThemeGTK's drawing of the button. It should be keeping its drawing inside the rect it's given.
(Unless it really needs to draw outside the rect, e.g. for focus rings, but then it needs to advertise that overflow. But I don't think that's the case here.)
(Reporter)

Comment 1

3 years ago
I'm not entirely sure what the white glow there actually is. Is it a button? Or a text field? Or just the shading of a button?
Can you link to the try run, please?
Is this intermittent or repeatable?

I haven't reproduced here, even with the patches on bug 1236043 and theme from
Ubuntu 12.04.

The drawing outside the bounds looks like the background of the button without
the border.  It looks the size of a full button.

I wonder whether there are other existing bugs in these results.  It seems they could be related.

Should max-width apply to the input elements or not?
If so, which wins between that and GetMinimumWidgetSize()?
Is type=image special?
Should the outline size be related to the element size, or are reset and
submit special?

I assume the button for type=file should not be narrower than both
GetMinimumWidgetSize() and max-width.

I assume flexbox doesn't clip, so these all seem to be issues in the sizing of
the elements, which seems related to size of the glow, but I don't know why the border is not present given the background is drawn.

I don't know why the left hand side of the glow is missing in the reference.  I don't expect incremental reflow (or incremental painting) on this short testcase.
(Reporter)

Comment 3

3 years ago
(In reply to Karl Tomlinson (ni?:karlt) from comment #2)
> Can you link to the try run, please?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6866731ab435&selectedJob=18834627

> Is this intermittent or repeatable?

Repeatable.

> I haven't reproduced here, even with the patches on bug 1236043 and theme
> from
> Ubuntu 12.04.

Thanks for testing! Maybe this is a bug in the specific theme that's used on the test machines?

> The drawing outside the bounds looks like the background of the button
> without
> the border.  It looks the size of a full button.

That's very strange.

> I wonder whether there are other existing bugs in these results.  It seems
> they could be related.

I'm not sure what you mean here. Are you referring to the results of the try push? There are some more reftest failures in there, but flexbox-widget-flex-items-3.html is the only one where the difference seems to be an actual bug. I think the other reftest failures are caused by existing bugs that make us paint slightly different pixel colors depending on the repainted region, and changing the way the invalid region gets simplified just exposes those existing bugs.

> Should max-width apply to the input elements or not?
> If so, which wins between that and GetMinimumWidgetSize()?
> Is type=image special?
> Should the outline size be related to the element size, or are reset and
> submit special?
> 
> I assume the button for type=file should not be narrower than both
> GetMinimumWidgetSize() and max-width.
> 
> I assume flexbox doesn't clip, so these all seem to be issues in the sizing
> of
> the elements, which seems related to size of the glow, but I don't know why
> the border is not present given the background is drawn.

I don't know the answers to these questions.

> I don't know why the left hand side of the glow is missing in the reference.
> I don't expect incremental reflow (or incremental painting) on this short
> testcase.

I haven't debugged this, but my guess is that we start out with a blank page, and when the actual test loads, we paint only the items that were added on top of the white background.
(In reply to Markus Stange [:mstange] from comment #3)
> (In reply to Karl Tomlinson (ni?:karlt) from comment #2)
> > Can you link to the try run, please?
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=6866731ab435&selectedJob=18834627

Thanks.  Repeatable, but not every time.

Linux opt R-e10s failed once and passed once.
There are no such failures with debug builds.

> Thanks for testing! Maybe this is a bug in the specific theme that's used on
> the test machines?

I was using the cairo and gtk libraries and theme from Ubuntu, which has
reproduced similar specific issues before.

It would be interesting to see whether it still occurs with a different theme.
https://hg.mozilla.org/try/rev/4cde64d71efa will change the theme.

> > I wonder whether there are other existing bugs in these results.  It seems
> > they could be related.
> 
> I'm not sure what you mean here. Are you referring to the results of the try
> push?

I meant this particular reftest.  Even without your changes, the rendering
looks very inconsistent to me.

> I haven't debugged this, but my guess is that we start out with a blank
> page, and when the actual test loads, we paint only the items that were
> added on top of the white background.

Ah, that's conceivable.
Daniel, could you look at the rendering in the attached log, please, and see questions in comment 2?

Even without the white glow on the file input, which is missing for me with and without Markus's patches, I find the inconsistencies in the rendering surprising.
Flags: needinfo?(dholbert)
(In reply to Karl Tomlinson (ni?:karlt) from comment #2)
> Should max-width apply to the input elements or not?

Yes, it should.

> If so, which wins between that and GetMinimumWidgetSize()?

GetMinimumWidgetSize() wins.  That's implemented here for flexbox:

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFlexContainerFrame.cpp?rev=6f91366b568a#1135

And it's implemented similarly somewhere for block layout, too, IIRC.

> Is type=image special?

It's not special -- it does actually end up 3px wide (honoring the max-width), just like everything else. But it looks wider because it has alt-text content ("Submit Query") which clearly overflows.  And that extends the outline, because the "outline" property wraps around all overflowing content (unlike "border").

(If you replace "outline" with "border", then you can see that this element ends up skinny just like every other element.)

> Should the outline size be related to the element size, or are reset and
> submit special?

Per above, the outline goes around the overflow area of the element. Reset and submit have a larger outline because they have a larger overflow area, from their textual contents (just like <input type="image"> does, from its textual contents).  They happen to clip their textual contents, though, so they perhaps shouldn't have that overflow area. This might be a bug in button code. I'll poke around a bit more and consider filing that.  That's orthogonal to this glow issue, though.

> I assume the button for type=file should not be narrower than both
> GetMinimumWidgetSize() and max-width.

Not sure.  That's all inside of the file control and depends on the file control's own layout code.

> I assume flexbox doesn't clip

Correct. [it does not clip its children. Note also that the reference case uses no flexbox code at all, and it renders basically the same as the testcase, only with a slightly different broken-glow effect. I'm assuming the difference there is random, given the first part of comment 4.)

> so these all seem to be issues in the sizing
> of the elements, which seems related to size of the glow, but I don't know
> why the border is not present given the background is drawn.

The outline (it's an outline, not a border) indicates the overflow areas -- so given that there's painting outside of the outline, that means the file control is calculating its overflow areas incorrectly -- it's painting outside of its reported overflow areas, which is a Bad Thing. That probably means we're not invalidating correctly, or something like that.
Flags: needinfo?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #6)
> The outline (it's an outline, not a border) indicates the overflow areas --
> so given that there's painting outside of the outline, that means the file
> control is calculating its overflow areas incorrectly

Or more likely: the overflow areas are correct, and our BuildDisplayList code is just painting outside of our reported overflow areas & not clipping appropriately, or something like that.

Some notes from poking around:
 - The file control is nsFileControlFrame, and it inherits its Reflow code directly from nsBlockFrame (a superclass), but it does *not* inherit block's BuildDisplayList code -- it's got its own custom (simple) BuildDisplayList impl (which just calls BuildDisplayListForInline).

I would bet that BuildDisplayListForInline isn't handling something correctly here.
(I filed bug 1261284 on the reset and submit issue mentioned in the middle of comment 6, BTW.)
Thanks, Daniel.  That's very helpful.

When the button is drawn by the native theme code, it draws the background and
border (or frame as GTK calls it - this is not outline) during one function
call.  Seeing a glow from the background without the border implies that the
native theme code is not drawing what it is told.  Whatever size it is told to
clip to, we should see the button border and background together.

The button, reset, and submit inputs, are using the same native widget drawing
as the file input, so I wonder what is different about the file input.  I
wonder whether the small size somehow triggers the bug in native theme drawing.

The width of the button, reset, and submit inputs seems to correspond to the
border from the widget code plus the padding from forms.css.

I would expect the file input to be at least as wide as the button input
because it contains an anonymous button.  It looks like nsFileControlFrame is
not considering the minimum size of its children.

Even linux Opt R1 is inconsistent about whether it is the reference or test that has the left hand part of the glow visible/missing.

Updated

2 years ago
Whiteboard: tpi:?

Updated

2 years ago
Keywords: polish
Priority: -- → P3
Whiteboard: tpi:? → tpi:+
You need to log in before you can comment on or make changes to this bug.