Last Comment Bug 715203 - -moz-column-fill property does not update from within script correctly
: -moz-column-fill property does not update from within script correctly
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: mozilla12
Assigned To: Scott Johnson (:jwir3)
: Jet Villegas (:jet)
Depends on:
Blocks: 715434
  Show dependency treegraph
Reported: 2012-01-04 10:29 PST by Scott Johnson (:jwir3)
Modified: 2012-01-10 01:52 PST (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

b715203 - Patch and test. (2.85 KB, patch)
2012-01-08 18:11 PST, Scott Johnson (:jwir3)
bzbarsky: review+
Details | Diff | Splinter Review

Description Scott Johnson (:jwir3) 2012-01-04 10:29:39 PST
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 [:bz] (still a bit busy) 2012-01-04 10:38:48 PST
Hmm.  We do have a reflow hint for column-fill.  Is that reflow happening?
Comment 2 Phil Ringnalda (:philor) 2012-01-04 10:40:32 PST
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) 2012-01-04 11:40:48 PST
(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) 2012-01-04 22:30:48 PST
Sure, but do you have reftests for ${whatever unexpected thing makes it not work on tbpl}? To pick one random possibility, 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) 2012-01-05 00:15:08 PST
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 [:bz] (still a bit busy) 2012-01-05 09:10:15 PST
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) 2012-01-05 10:41:49 PST
Yep, that plus bug 715434 does indeed fix tbpl.
Comment 8 Scott Johnson (:jwir3) 2012-01-07 12:39:26 PST
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 [:bz] (still a bit busy) 2012-01-07 20:02:41 PST
Use any value of "overflow" other than "visible", pretty much.  See comment 5, for example.
Comment 10 Scott Johnson (:jwir3) 2012-01-08 18:11:02 PST
Created attachment 586867 [details] [diff] [review]
b715203 - Patch and test.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2012-01-08 18:23:13 PST
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) 2012-01-09 09:28:28 PST
(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 [:bz] (still a bit busy) 2012-01-09 09:31:36 PST
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) 2012-01-09 09:45:15 PST
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) 2012-01-09 14:16:50 PST
Pushed to inbound:
Comment 16 Marco Bonardo [::mak] 2012-01-10 01:52:40 PST

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