Closed Bug 1130007 Opened 6 years ago Closed 6 years ago

in-content preferences: regression: misaligned homepage placeholder

Categories

(Firefox :: Theme, defect, P2)

All
macOS
defect
Points:
1

Tracking

()

VERIFIED FIXED
Firefox 38
Iteration:
38.2 - 9 Feb
Tracking Status
firefox38 --- verified

People

(Reporter: soeren.hentzschel, Assigned: Gijs)

References

Details

(Keywords: regression)

Attachments

(2 files)

Attached image screenshot
There is a regression in the latest Nightly with the vertical alignment of the homepage label in the in-content preferences. Furthermore the input field has a different width compared with the select box above the homepage field.
"label" should read "placeholder text"…
Summary: in-content preferences: regression: misaligned homepage label → in-content preferences: regression: misaligned homepage placeholder
When did this last work?
Blocks: 718011, ship-incontent-prefs
No longer blocks: 1014208
Flags: needinfo?(cadeyrn)
I'm guessing bug 1038291 broke this, but I'm not sure.
This is because the textbox inside the table is now display: block and no more display: -moz-box. On Linux and Windows this change makes the text jumping 1px up, on OS X it's around 3px.

I could add a padding-top: 3px to fix this, but would this be okay and clean? Or is there a other solution?
(In reply to Richard Marti (:Paenglab) from comment #4)
> This is because the textbox inside the table is now display: block and no
> more display: -moz-box. On Linux and Windows this change makes the text
> jumping 1px up, on OS X it's around 3px.
> 
> I could add a padding-top: 3px to fix this, but would this be okay and
> clean? Or is there a other solution?

Why can't we just make it display: -moz-box ?
(In reply to :Gijs Kruitbosch from comment #5)
> 
> Why can't we just make it display: -moz-box ?

This was my first reaction too, but this didn't work (also with !important) it was still display: block.
(In reply to Richard Marti (:Paenglab) from comment #6)
> (In reply to :Gijs Kruitbosch from comment #5)
> > 
> > Why can't we just make it display: -moz-box ?
> 
> This was my first reaction too, but this didn't work (also with !important)
> it was still display: block.

It's because the parent (the table cell) is using display: flex. Why is that necessary?

I'll also note that this generally looks sloppy on OS X - the action dropdown for "When Nightly starts" and the buttons/textboxes below and above it are misaligned horizontally on the end side, for example.

Was this tested at all on OS X ? :-(
Depends on: 1038291
Flags: needinfo?(cadeyrn)
Priority: -- → P2
(In reply to :Gijs Kruitbosch from comment #7)
> (In reply to Richard Marti (:Paenglab) from comment #6)
> > (In reply to :Gijs Kruitbosch from comment #5)
> > > 
> > > Why can't we just make it display: -moz-box ?
> > 
> > This was my first reaction too, but this didn't work (also with !important)
> > it was still display: block.
> 
> It's because the parent (the table cell) is using display: flex. Why is that
> necessary?

The flex is needed to stretch the cell to the whole width.
(In reply to Richard Marti (:Paenglab) from comment #8)
> (In reply to :Gijs Kruitbosch from comment #7)
> > (In reply to Richard Marti (:Paenglab) from comment #6)
> > > (In reply to :Gijs Kruitbosch from comment #5)
> > > > 
> > > > Why can't we just make it display: -moz-box ?
> > > 
> > > This was my first reaction too, but this didn't work (also with !important)
> > > it was still display: block.
> > 
> > It's because the parent (the table cell) is using display: flex. Why is that
> > necessary?
> 
> The flex is needed to stretch the cell to the whole width.

display: flex just sets it up to be a flex container. Considering that we're in XUL layout and you could just give the textbox -moz-box-flex (or attribute flex="1" or whatever), I still don't really understand why this is necessary.
(and anyway, if the table itself is forced to a certain width, cells normally fill up the space in the table... why is that not enough here?)
(In reply to :Gijs Kruitbosch from comment #9)
> 
> display: flex just sets it up to be a flex container.

Yes, but the elements in this container are using flex-grow: 1; to have a working wrapping of the three buttons below.

> Considering that we're in XUL layout and you could just give the textbox
> -moz-box-flex (or attribute flex="1" or whatever), I still don't really
> understand why this is necessary.

Is this still a normal XUL layout with the table in it? It's not a grid, for why please read bug 1038291.
Can you look at setting width:100% on the menulist and then setting display:flex on the table cell that has the three buttons?
This works on OS X. I'll test Windows in a second.
Attachment #8560451 - Flags: review?(jaws)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment on attachment 8560451 [details] [diff] [review]
fix layout of general pane on OS X,

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

Yep, looks good on Windows.
Attachment #8560451 - Flags: review?(jaws) → review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/73ea902639a7
Iteration: --- → 38.2 - 9 Feb
Points: --- → 1
Flags: qe-verify-
Flags: in-testsuite-
Flags: firefox-backlog+
display: -moz-box; and -moz-box-flex: 1; works as well (instead of display: flex; and flex-grow: 1;), plus it's adapted to xul.
Not when the buttons should wrap when the text in them is to wide.
https://hg.mozilla.org/mozilla-central/rev/73ea902639a7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Verified fixed on latest Nightly 39.0a1 (buildID: 20150317030206) and latest Aurora 38.0a2 (buildID: 20150316004007).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.