Opened 11 years ago

Closed 11 years ago

Last modified 10 years ago

#8864 closed defect (fixed)

[patch][ccla] Views not lining up properly in the DataGrid (FF3)

Reported by: Jared Jurkiewicz Owned by: Bryan Forbes
Priority: high Milestone: 1.4
Component: DojoX Grid Version: 1.2.3
Keywords: Cc:
Blocked By: Blocking:

Description

This can be seen on Firefox 3 using the test at http://download.dojotoolkit.org/release-1.2.3/dojo-release-1.2.3/dojox/grid/tests/test_grid_column_display.html

This can be tricky to reproduce but by messing with the font size (Ctrl++ and Ctrl+-) and using the vertical scroll bar at various times you can achieve the result seen in the attached image. The first view which is set as 'noscroll' does not line up with the second view which is scrollable.

Attachments (4)

view_alignment_problem.jpg (251.7 KB) - added by Jared Jurkiewicz 11 years ago.
grid_rowheight_ff3.patch (448 bytes) - added by Jared Jurkiewicz 11 years ago.
Patch for odd misalignment issue in FF3.
grid_rowheight_ff3_andcompat.patch (1023 bytes) - added by Jared Jurkiewicz 11 years ago.
Slightly better patch. This also fixes the same issue in the backwards compat grid!
updated.patch (1.1 KB) - added by Jared Jurkiewicz 11 years ago.
Updatedpatch (line nos)

Download all attachments as: .zip

Change History (14)

Changed 11 years ago by Jared Jurkiewicz

Attachment: view_alignment_problem.jpg added

comment:1 Changed 11 years ago by Bryan Forbes

Milestone: tbdfuture

comment:2 Changed 11 years ago by Jared Jurkiewicz

Still exists in 1.3

comment:3 Changed 11 years ago by Jared Jurkiewicz

This is easily reproducible. Just increase font size 2x from default (ctrl+ twice), then scroll. The glitching happens 100% of the time when I try that.

comment:4 Changed 11 years ago by Jared Jurkiewicz

Okay, after digging around, I located the function that handles computing row height. It's in:

_ViewManager.js

Function: normalizeRowNodeHeight

What I determined was this function sets the height on the table that represents the row after it goes through each cell and determines the max cell height. It even does this across views, which is awesome.

What I discovered on FF3 is interesting. When I increased font size twice (ctrl++), I would get the offset rendering. The heights of cells that had text wrappiing on the right were always *slightly* larger than the cells on the left visually.

...

But here's the odd part, when I looked in the Firebug inspector, the height on the tables representing each part of the row *all* had the same CSS height set, 35px. So ... why in the world are they rendering visually different?

On a lark I put a test in there that if the browser is ff3 and h > 0, add 1px to h. And guess what? This *fixed* the rendering. Now they always align.

This leads me to think there is a bug in dojo.marginBox on FF3, or just a bug in FF3 in how it returns the margin box of the target node. What I think is happening is that the calculation of the margin box ALWAYS rounds down, even if say it claimed the margin box was 35.7px, Grid would be told 35px. So then the cell really is just *slightly* larger than its claim and when rendered ends up showing this, even with the CSS height set to 35px.

By adding 1, I account for the round-down effect and now it always is of 'the right size' when the CSS height is applied. In the worst case, it makes the cells 1px taller than then need be or so.

Anyway, that's my guess as to the cause and the patch for this is insanely simple. It'll be attached shortly.

Changed 11 years ago by Jared Jurkiewicz

Attachment: grid_rowheight_ff3.patch added

Patch for odd misalignment issue in FF3.

comment:5 Changed 11 years ago by Jared Jurkiewicz

Summary: Views not lining up properly in the DataGrid[patch][ccla] Views not lining up properly in the DataGrid

Changed 11 years ago by Jared Jurkiewicz

Slightly better patch. This also fixes the same issue in the backwards compat grid!

comment:6 Changed 11 years ago by Jared Jurkiewicz

Here's what we've been able to determine (Bill Keese helped). In FF3, when ctrl++ is used, sometimes you actually get fractional font sizes, like 48.4667px. I don't see how that's even remotely sane, but such is what it is. When marginBox fires on FF3, it's just using standard FF functions (getComputedStyle, etc), there really isn't much of a branch for mozilla (nothing for width and height).

Everything marginBox returns comes back in full integers. So it looks like Firefox itself is truncating off (or rounding down), that font size when it returns box size. This turns out to be th wrong thing to do, as the font still ends up using that px, so what it ought to really be doing is rounding up.

Anyway, I talked with Bill Keese a fair amount on this, and his opinion was it wasn't worth the risk of trying to 'fix' Firefox in marginBox(), because in most all normal cases, it works fine and changing it poses a larger threat to the dojo system as a whole. Since grid is the only place that currently exhibits this causing a real layout problem and since it's a one-liner to work around (yay browser bugs), it's safer for dojo if we just patch grid for this FF3 oddity with my one-line patch.

I tend to agree with him on this. If we can come up with a marginBox fix down the road, awesome, but I'm not convinced its worth messing with marginBox at this time if this is the only area the problem makes itself apparent.

I'd like to just go ahead and check this in now as a start fix (we can leave this tracker open if you want, if something better comes along that fixes marginBox, and at least get moving on the issue. :-)

comment:7 Changed 11 years ago by bill

Summary: [patch][ccla] Views not lining up properly in the DataGrid[patch][ccla] Views not lining up properly in the DataGrid (FF3)

Yup, plus which this is no longer a problem on FF3.5 beta, so the smallest fix is the best (if anything).

Note that in firebug's computed-style tab the height of the <td> is also fractional, although node.offsetHeight is an integer (hence why marginBox() returns an integer).

Changed 11 years ago by Jared Jurkiewicz

Attachment: updated.patch added

Updatedpatch (line nos)

comment:8 Changed 11 years ago by Jared Jurkiewicz

Resolution: fixed
Status: newclosed

(In [17358]) Checking in small tweak to fix FF3 with grid resize. \!strict fixes #8864

comment:9 Changed 10 years ago by Adam Peller

Milestone: future1.4

comment:10 Changed 10 years ago by Adam Peller

(In [20431]) Remove FF2 support. Refs #9183, #8864

Note: See TracTickets for help on using tickets.