118.21 KB, image/jpeg
48.60 KB, image/jpeg
2.01 KB, patch
|Details | Diff | Splinter Review|
1.24 KB, patch
Mats Palmgren (vacation - back in August): review+
|Details | Diff | Splinter Review|
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4 When printing out a web page that has a combo box on it, the text in the combo box is cut off after the first whitespace. Reproducible: Always Steps to Reproduce: 1. Go to a page that has a combo box with an item having a space in it. (An example is at http://forums.beyondunreal.com/forumdisplay.php?f=12, at the bottom of the page) 2. Select that item, if it is not already selected. 3. Print the page. Actual Results: The text in the combo box only shows the first word of the item. Expected Results: The entire combo box text should be printed.
We can reproduce with intl page. If select elements have non-ASCII character in its label, the label's first character is only printed. http://bugzilla.mozilla.gr.jp/query.cgi This can be reproduced on Mac too.
1.8 branch has this bug. I think that this is serious problem for printing. An Japanese tester said: Firefox/2005062006-trunk/WinXP O.K. Firefox/2005070106-trunk/WinXP buggy.
FYI: This bug is reproduced on Print Preview.
Is this a regression? Or something we've always had?
See comment 4. We have a report that said 2005062006 build doesn't have this bug.
Ah, ok. This regressed between 2005-06-20 06:00 and 2005-06-21 06:00. Bonsai query: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=20050620+060000&maxdate=20050621+060000&cvsroot=%2Fcvsroot
My guess is this is fallout from bug 297389 somehow. That 'Y' is the first Unichar in the string cast to char, but I have no idea how _that_ happens...
One possibility might be that we post the event, but then disconnect from the document ('cause printing is like that), so the text change when the event fires is not propagated to the frame and as a result the frame has bogus offsets as compared to the actual text in the content node....
Mats, if you can come up with a low risk patch that fixes this in the next few days, we'd consider taking it for the release but this isn't going to block.
We might be able to work around this with that "spin an event loop till all pending PLEvents are done for printing" hack I recall we had at some point... shaver, do you happen to know what file that was in?
Created attachment 200171 [details] [diff] [review] Patch that would work if printing sucked less Unfortunately, the page sequence frame ignores all but the initial reflow. After that it never reflows its kids. So the reflow that would update our textframes is stopped before it ever gets to them. I can't make enough sense of the mess that is nsSimplePageSequenceFrame at this time of night. I might be able to look at this before Sunday, but I really doubt it. Help with this would be much appreciated... :(
Created attachment 200256 [details] [diff] [review] Proposed fix Just get the right text up front when we're creating the content. That way we don't have to worry about having to change the text from reflow and then having to do a second reflow... This should be quite safe, but if people are worried I'm willing to set the index back to -1 and Truncate() mDisplayedOptionText after calling ActuallyDisplayText. I really don't think that's needed, though. I did verify that we're guaranteed a mListControlFrame by the time we get here.
Comment on attachment 200256 [details] [diff] [review] Proposed fix Mats, if you get a chance to review too, that would be great.
Fixed on trunk.
Comment on attachment 200256 [details] [diff] [review] Proposed fix Requesting branch approval. This just sets the text to the right thing up front before layout has happened, so we don't need a second pass of reflow. This should be quite safe, and fixes a problem where our textframes are basically out of sync with our content nodes and reading data from the latter in a way inconsistent with the way that data is actually stored.
bz, we can consider this for the branch after it's been verified on the trunk. What kind of testing do we need to do on the trunk to feel confident about taking this (what could break?) Is it all in printing or could there be page layout regressions?
The thing to test would be comboboxes. Those should be the only thing affected. The thing to really test would be comboboxes that get inserted into the DOM (which constructs the frame and runs the code I'm changing) and then have the selected item modified by script before they actually lay out. Once we lay them out the first time, we set the selected index and text (in Reflow()), and then this patch no longer matters. I did do some testing of that sort of thing, and it all worked. But if others can test too, that would be great. That's as far as page layout regressions go. For printing, the thing to test is printing of comboboxes, but here of course there is no DOM mutation involved. I'm not even sure what to suggest to test; that is, I can't think of things that might break.
selects print ok for me with a Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051021 Firefox/1.6a1 cvs build.
Verified using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051021 Firefox/1.6a1. I printed out the URL cited and the page prints fine, nothing gets cut off.
Not really sure what to test here. It seems to work fine on various websites. I've made sort of a testcase for this: http://wargers.org/mozilla/bug311380/selectindex.htm Also works just fine.
Sorry for the bug spam, but I thought the drivers should see this from a productivity perspective. Today, I had printed something off to be turned in but all the combo boxes were messed up. It's kinda annoying when I'm trying to print a completed application off to be turned in and all the boxes only have the first word in them. :) It would definitely be nice to get this fixed for 1.8. ;)
Fixed on branch.
Patch was granted approval1.8rc1, so re-requesting blocking1.8rc1 for tracking purposes.
There's nothing to track. This is not blocking 1.8rc1. But it's checked in on the 1.8 branch, so it'll make 1.8rc1.
Ah, I thought you all wanted to have all the 1.8rc1 bugs to have blocking1.8rc1 too (for some type of tracking). My fault. :) Sorry for wasting your time.
grr, stupid bugzilla :/ Can somebody minus it again? Mid-air collision messed this up.
This is why you don't just blindly click the "overwrite other people's changes" button in a mid-air...
Well, I called it a mid-air collision, but I guess it wasn't in the definition of the phrase, though it should have been. After I got the e-mail denying blocking, I refreshed the page and added my comment. Somehow I guess it hadn't minused it yet as it re-requested blocking1.8rc1? even though I didn't set it that time. Oh well.
> I refreshed the page and added my comment. Use shift-reload when doing that please. reload preserves form state, even if it has changed on the server.
*** Bug 314333 has been marked as a duplicate of this bug. ***
Comment on attachment 200256 [details] [diff] [review] Proposed fix Boris, the patch looks good, but I also think we should fix ReflowPrintObject() like you outlined in your first patch. I think posting events is a good way to avoid recursive frame construction and the way ReflowPrintObject currently works blocks that pattern from being used more.
The problem with that patch is that it doesn't work because of http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsSimplePageSequence.cpp&rev=3.136&mark=287-291#286 and http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsSimplePageSequence.cpp&rev=3.136&mark=208-217#207
Though some of that code is going away on the reflow branch... so maybe after that lands we should revisit this.... But note that we nuked FlushEventQueue() in bug 325991 because it was causing problems.