Last Comment Bug 680518 - progress element display:block update issue
: progress element display:block update issue
Status: VERIFIED FIXED
[qa!]
: testcase
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on:
Blocks: 686886
  Show dependency treegraph
 
Reported: 2011-08-19 12:01 PDT by François Kooman
Modified: 2011-10-05 05:28 PDT (History)
5 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
wontfix
wontfix
fixed
fixed


Attachments
reporter's testcase (2.00 KB, text/html; charset=UTF-8)
2011-08-19 20:54 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details
progress element in table flows out of column (523 bytes, text/html)
2011-08-20 09:01 PDT, François Kooman
no flags Details
Patch v1 (3.03 KB, patch)
2011-08-22 01:39 PDT, Mounir Lamouri (:mounir)
roc: review+
bugzilla: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description François Kooman 2011-08-19 12:01:51 PDT
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0) Gecko/20100101 Firefox/6.0
Build ID: 20110811165603

Steps to reproduce:

Use the new HTML5 progress element and added style for the progress element:

progress {
  display: block;
}


(Used demo from http://thebrowsereview.com/demos/progress/progress.html).

Tested with nightly Linux 64-bit Intel Built on 19-Aug-2011.



Actual results:

Using Firefox 6 (and later also nightly 64 bit from 19-Aug-2011):

- Progress bar is nicely updated on Mac OS X
- Progress bar does not update on Linux and Windows

(The progress bar does update on Linux and Windows when the Firefox window gets resized (manually).


Expected results:

Progress bar should also update on Linux and Windows.

Work around: remove the display: block style.
Comment 1 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-08-19 13:59:42 PDT
http://thebrowsereview.com/demos/progress/progress.html works fine for me (ticks up each second) on 64-bit Linux, on both 6.0 release and on nightly.
Comment 2 Matthias Versen [:Matti] 2011-08-19 14:23:45 PDT
The demo url is wfm with Mozilla/5.0 (Windows NT 6.1; rv:9.0a1) Gecko/20110818 Firefox/9.0a1 SeaMonkey/2.6a1
Comment 3 François Kooman 2011-08-19 14:48:15 PDT
Yes, the demo works as intended. The problem starts to occur once you add the style:

progress {
  display: block;
}

To the CSS of the page.
Comment 4 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-08-19 20:51:12 PDT
Please attach a standalone testcase to the bug.  The demo requires other resources on your site (jQuery).
Comment 5 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-08-19 20:52:43 PDT
Actually, never mind; it looks like it just includes jQuery but doesn't actually need it; I'll attach.
Comment 6 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-08-19 20:54:35 PDT
Created attachment 554604 [details]
reporter's testcase

http://thebrowsereview.com/demos/progress/progress.html with the on-site jQuery links at the bottom removed, and with progress { display: block } added.
Comment 7 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-08-19 20:55:39 PDT
Confirmed on Linux.
Comment 8 François Kooman 2011-08-20 09:01:15 PDT
Created attachment 554641 [details]
progress element in table flows out of column

Another related? layout bug with the progress element. The progress bar flows out of its table cell if no explicit width is specified. 

Works fine on: Google Chrome Dev channel and Webkit nightly (Mac OS X)
Fails on Firefox 6 (Mac OS X), Firefox 64 bit Nightly on Linux...
Comment 9 Mounir Lamouri (:mounir) 2011-08-21 03:35:53 PDT
(In reply to F. Kooman from comment #8)
> Created attachment 554641 [details]
> progress element in table flows out of column
> 
> Another related? layout bug with the progress element. The progress bar
> flows out of its table cell if no explicit width is specified. 
> 
> Works fine on: Google Chrome Dev channel and Webkit nightly (Mac OS X)
> Fails on Firefox 6 (Mac OS X), Firefox 64 bit Nightly on Linux...

Could you open another bug for that? I do not think this is related to the original issue.
Comment 10 François Kooman 2011-08-21 03:42:35 PDT
Another bug opened as requested for the progress element flowing out of the table cell: https://bugzilla.mozilla.org/show_bug.cgi?id=680747
Comment 11 Mounir Lamouri (:mounir) 2011-08-21 04:06:07 PDT
So, usually, progress bars have two elements: the container and the bar itself. This is not the case when using the MacOS X native rendering: the container draws the bar. If it works on MacOS X and not on other platforms, the reason must be related to that.

I thought maybe the frame wasn't reflowed correctly but both are according to my testings. I thought reflowing was invalidating the frames and when the frame was invalidated it was repainted but I guess I must be wrong, otherwise it would be working as expected :)
David, do you have any hints about what might be happening?
Comment 12 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2011-08-21 11:45:03 PDT
Yeah, something needs to invalidate something.  I think roc remembers the rules.  (When it's inline, blocks tend to invalidate a lot, so the bug was being hidden.)
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-21 16:00:11 PDT
The basic rules are:
-- if the parent frame moves the child, it must completely invalidate the old and new overflow areas
-- if the parent frame resizes the child without moving it, it must completely invalidate the difference between the old and new border-boxes
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-08-21 16:03:27 PDT
Probably the best thing to do is, whenever something changes that could affect the result of GetPosition, just invalidate the entire nsProgressFrame.
Comment 15 Mounir Lamouri (:mounir) 2011-08-22 01:39:39 PDT
Created attachment 554803 [details] [diff] [review]
Patch v1

Roc, thank you for the explanations. I think this patch corresponds to what you did recommend and fixes this issue.
Comment 16 Mounir Lamouri (:mounir) 2011-08-22 10:57:16 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/4b1410af17c7
Comment 17 Mounir Lamouri (:mounir) 2011-08-22 11:01:01 PDT
Comment on attachment 554803 [details] [diff] [review]
Patch v1

This is web facing so it would be great to have this pushed to aurora to reduce the amount of time this bug will be visible.
In addition. it's really non-risky.
Comment 18 Johnathan Nightingale [:johnath] 2011-08-23 14:59:34 PDT
Comment on attachment 554803 [details] [diff] [review]
Patch v1

Approved for aurora, assuming that we triple check that the added invalidation didn't move any of our performance needles (I wouldn't expect this to be a hot path on pageload, but I am also not a layout hacker)
Comment 19 Mounir Lamouri (:mounir) 2011-08-24 03:43:48 PDT
Pushed to Aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/d6ce33ded6a1
Comment 20 Mounir Lamouri (:mounir) 2011-08-24 03:52:25 PDT
François, and other interested, the fix is going to happen in Firefox 8 which is going to be released in about 3 months.
Sorry for the inconvenience.
Comment 21 Virgil Dicu [:virgil] [QA] 2011-10-05 00:21:24 PDT
Mozilla/5.0 (X11; Linux x86_64; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (X11; Linux x86_64; rv:9.0a2) Gecko/20111003 Firefox/9.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:10.0a1) Gecko/20111002 Firefox/10.0a1

Verified in Firefox 8,9,10 on Ubuntu 11.04, Mac OS 10.6, Windows Xp and Windows 7 using the two test cases provided in the attachments.

The progress block is displayed as expected and the the table no longer flows out of the column.

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