715203 - -moz-column-fill property does not update from within script correctly

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Scott Johnson (:jwir3)]

Viewing tbpl errors (example given by philor was a ts failure on android), the results section in the lower right hand corner is now set to 'balance', when it was previously 'auto', prior to the landing of bug 695222. This is expected, since the spec dictates that we should default to 'balance'. However, when attempting to do:

document.getElementById('results').style.MozColumnFill = "auto"

No visible change happens to the results div element. Similarly, a follow-up:

window.getComputedStyle(document.getElementById('results')).getPropertyValue('-moz-column-fill') shows 'auto', as expected, but, again, no change is visible.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hmm.  We do have a reflow hint for column-fill.  Is that reflow happening?

Comment 2

[Phil Ringnalda (:philor)]

I don't think it's just "from script" - my dev copy of tbpl has -moz-column-fill: auto; in the stylesheet, and it still gets balanced.

Comment 3

[Scott Johnson (:jwir3)]

(In reply to Phil Ringnalda (:philor) from comment #2)
> I don't think it's just "from script" - my dev copy of tbpl has
> -moz-column-fill: auto; in the stylesheet, and it still gets balanced.

That's really strange, because there are unit tests that check this. Specifically, there are reftests that check that the behavior of column-fill is different on auto than balance (layout/reftests/columns/columnfill-auto and columnfill-balance), as well as checking the behavior is correct for both auto and balance. Additionally, there's a reftest that checks to see if changing the column-fill property has an effect on the layout (layout/reftests/columns/columnfill-change). AFAICT, these aren't failing...

Comment 4

[Phil Ringnalda (:philor)]

Sure, but do you have reftests for ${whatever unexpected thing makes it not work on tbpl}? To pick one random possibility, http://mxr.mozilla.org/mozilla-central/search?find=%2Flayout%2Freftests%2Fcolumns%2F&string=%3Cul says there's not a single ul in the columns reftests, and I'll bet there aren't any that fill the column by setting the .innerHTML, which I think is what tbpl does.

Comment 5

[Phil Ringnalda (:philor)]

Maybe minimized version of tbpl (maybe I have no idea what I'm doing, too): 

data:text/html,<div style="-moz-column-width: 40em; overflow-x: auto; -moz-column-fill: auto;"><li>one<li>two<li>three</div>

Without the overflow-x: auto, that's one column; with, it's three.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Ah, yes.  For scrollable columnated stuff, the column styles are on the scrollframe, not on the columnset.

If you look at the ua.css rules for -moz-scrolled-*, they explicitly inherit column-count, column-width, column-gap, and column-rule.  Presumably they need to also inherit column-fill.

Comment 7

[Phil Ringnalda (:philor)]

Yep, that plus bug 715434 does indeed fix tbpl.

Comment 8

[Scott Johnson (:jwir3)]

Ok, so I fixed this in a patch, but I'd also like to write a quick reftest. How do I invoke this -moz-scrolled-content property? I can't seem to find any useful documentation on it...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Use any value of "overflow" other than "visible", pretty much.  See comment 5, for example.

Comment 10

[Scott Johnson (:jwir3)]

Attachment: b715203 - Patch and test.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 586867 [details] [diff] [review]
b715203 - Patch and test.

Probably better to call the test file something like column-fill-with-overflow-style.html or something.

And does this need to be an HTTP test?  If it really does, maybe document why?

r=me with that.

Comment 12

[Scott Johnson (:jwir3)]

(In reply to Boris Zbarsky (:bz) from comment #11)

> And does this need to be an HTTP test?  If it really does, maybe document
> why?
Yes, actually. It uses the ahem font, which is located in layout/reftests/fonts. I'll document it.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Hmm.  It doesn't have @font-face rules for that font, so why is HTTP involved?  (I might just be misunderstanding part of the test harness setup, of course.)

Comment 14

[Scott Johnson (:jwir3)]

Ah, yes, you are correct. I neglected to add that rule, and it was still passing. I've removed the font-family from the html files, and HTTP from reftest.list. :)

Comment 15

[Scott Johnson (:jwir3)]

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3244d19894c

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/d3244d19894c