366722 - make non-boxes support nsITheme::GetMinimumWidgetSize

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Currently only nsBox and nsTreeBodyFrame (for twisties) call nsITheme::GetMinimumWidgetSize.  However, Josh needs this for an nsITheme implementation of checkboxes on the Mac, so that he can make the checkboxes the right size without forms.css hacks (which could in turn be overridden by authors).

This might cause some existing code that never did anything to suddenly start working, so it could break some things, but if it does we can fix the existing code.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Don't you need to change GetMinWidth and GetPrefWidth too?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yeah.  This also may be border-box width rather than content-box width -- I need to investigate that further.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Although it's not clear from the nsBox.cpp code (and I think box code sometimes uses the width/height properties as border-box), it's clear from nsCSSRendering that these sizes should be border-box.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We should probably have GetWidgetOverflow in here too.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Handled overflow, by moving the handling of GetWidgetOverflow from nsBox's call to FinishAndStoreOverflow to inside FinishAndStoreOverflow.  I also fixed a bunch of comments in nsITheme.h

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to trunk.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: pay better attention to borders

This might help some problems josh is having with borders.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Wouldn't we need something similar for padding?

Should we store the padding and borders in the properties, perhaps, for efficiency and less code duplication?

Does this fix bug 367320?

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: pay better attention to borders

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What do you think about putting theme borders/padding in the properties?

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I was thinking about that.  We'd have to inhibit rule tree sharing (which would be a space/perf loss), but it would probably make things a good bit simpler.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sorry, I don't mean the style properties, but the frame properties that you use to store percentage padding/borders

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: pay better attention to borders and padding

The only change relative to the last patch is changing GetUsedPadding too.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: pay better attention to borders and padding

I'm hoping this will fix the bad-layout aspects of the regressions.

However, we really need to figure out a better way for the windows native theme code to decide whether to pass TS_TRUE or TS_MIN.  Basically GetMinimumWidgetSize on Windows in many cases returns the Windows theme API's pref size rather than its min size.  I'm not sure whether we want to extend the theme API to report both pref size and min size or whether we want the nsITheme code to do one or the other based on whether the frame is an HTML form control or not.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 252939 [details] [diff] [review]
pay better attention to borders and padding

This fixes the regressions from the previous patch based on the testcases in bug 367320 and bug 368258.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 252939 [details] [diff] [review]
pay better attention to borders and padding

looks OK, but I still want to know whether it would be simpler to just store widget padding and border in frame properties the way we do for percentage padding.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't think it would be much simpler, since we'd have to find an earlier time to store the properties and we'd still have to check IsThemed all over the place.  Common functions to get the padding and border from the theme might help more (although again not all that much).

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Anyway, above patch checked in.