Closed Bug 613696 Opened 14 years ago Closed 14 years ago

Minefield button randomly changes sizes.

Categories

(Core :: Layout: Text and Fonts, defect)

Product:
Core
Component:
Layout: Text and Fonts
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- final+

People

(Reporter: ben.r.xiao, Assigned: roc)

References

Details

(Whiteboard: [hardblocker](?))

Attachments

(5 files)

screenshot
30.50 KB, image/jpeg
Details
A clearer screenshot of several states stacked over each other.
5.34 KB, image/png
Details
Screen capture of the button changing size on hover
87.19 KB, video/ogg
Details
AutoIt3 script reproducing the bug
429 bytes, text/plain
Details
fix
20.23 KB, patch
dbaron
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101119 Firefox/4.0b8pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101119 Firefox/4.0b8pre

Sometimes the Minefield button changes size on hover. This happens the most when the screen resolution is changed (etc. while gaming). When the problem occurs, you first see that the Minefield text becomes more narrow, all the letters are squeezed together slightly. When you hover over the button, it changes size horizontally to accomodate the narrower text.

Reproducible: Sometimes

Steps to Reproduce:
1. Open Minefield
2. Change resolution (you might need to do this multiple times before the problem occurs)
3. See that the letters in "Minefield" is squeezed together slightly.
4. Hover over the button and watch it change in size
Actual Results:  
Minefield button changes horizontal size randomly.

Expected Results:  
Minefield button does not change in size.
Attached image screenshot — 
I can confirm this on Win Vista. The labeling of the tabs is also affected. I am quite sure that this is a regression. I do not have a reliable STR but it is not necessary to change the resolution, it can also happen after opening / closing tabs or minimizing firefox.
Yes it is not necessary to change the resolution. It happens randomly, but I find the most reliable way to reproduce the bug is by switching resolutions. Otherwise the only other way is just to wait :(
I've been seeing this, as has :davida, we were both waiting for reliable STR.

Seems to be related to the font size in the button. On hover, it changes. Once it's changed, it stays that way for a while, then goes back?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Wondering if this might be related to bug 588880?
I suspect this does have to do with hardware accel.  I don't ever see this on my laptop which was blacklisted from all hardware acceleration but I have seen it on my wife's machine which isn't blacklisted.
I would be - is there a stronger word than flabbergasted? - struck deaf, dumb and blind by sheer astonishment if this had anything to do with hardware acceleration.

However, it almost certainly has to do with DirectWrite, which is often conflated with hardware acceleration. Can someone confirm that for me?
@joe: how can we help confirm that?
You can disable direct write in about:config and see if the problem persists. 

Joe, you're right. I was conflating the two. DirectWrite, not HW Accel.
I wasn't able to find a way to disable DirectWrite via about:config. This definitely happens to me. This bug seems like it belongs in a Core components. I'm on Vista...

Mozilla/5.0 (Windows NT 6.0; rv:2.0b8pre) Gecko/20101130 Firefox/4.0b8pre
blocking2.0: --- → ?
Component: Theme → Layout: Text
OS: Windows 7 → All
Product: Firefox → Core
QA Contact: theme → layout.fonts-and-text
Version: unspecified → Trunk
gfx.font_rendering.directwrite.enabled
blocking2.0: ? → final+
(In reply to comment #10)
> gfx.font_rendering.directwrite.enabled

Doesn't work. It's set to "disabled" but directwrite is still enabled (as seen on about:support).
You'll also have to ensure that D2D is not force-enabled, and that D3D9 layers are being used instead of D3D10. D3D10 requires D2D which requires DirectWrite...
Ok, so essentially the pref to change is gfx.direct2d.disabled. I am testing with this now.
I did a bit of regression testing (with no actual STR though):
works (?):
win32 2010-11-14-04-mozilla-central 674f2ed15cea
broken:
win32 2010-11-15-04-mozilla-central edf41ff32f08

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=674f2ed15cea&tochange=edf41ff32f08

(Though I've got the feeling that the bug happens more often in current builds.)
Although the button just jumps a few pixels, it's more distracting than it sounds because it occurs exactly when your mouse is over it and your attention on the button.
There is a STR in bug 616384.
There's a lot of good information in bug 616384, actually.
Keywords: regressionwindow-wanted
I'm seeing this bug pretty constantly throughout the day.  Here's a quick screen capture of the issue.
I attached an AutoIt3 script which reproduces the issue. It can run forever and one can see the button changing back and forth. (This is much simpler than the STR in bug 616384.)

To use it just download and install AutoIt3: http://www.autoitscript.com/autoit3/downloads.shtml, then load the script in the SciTE Script Editor, then press Tools|Go.
Attachment #498779 - Attachment mime type: application/octet-stream → text/plain
I've been looking into this a bit, and it appears that the text of the "Minefield" button is sometimes rendered to a D2D surface, and sometimes to an image surface. When it is rendered to a D2D surface, the cairo scaled_font has its hint_metrics flag set to CAIRO_HINT_METRICS_DEFAULT, but when it is rendered to an image surface, the hint_metrics flag is set to CAIRO_HINT_METRICS_ON. This results in differing glyph metrics, because when CAIRO_HINT_METRICS_ON is used, the harfbuzz shaper code rounds the "ideal" metrics it computes from the raw font data.

The D2D-surface call seems to occur when the textrun is constructed during reflow, presumably for the purpose of measuring the text, while the image-surface case occurs when the textrun is being (re)constructed during layer rendering. Presumably this can happen if the old textrun has expired from the cache, and then the next operation that needs it is a layer-drawing operation.
Sounds right.

But in all cases I would have thought the context involved would be the result of PresShell::GetReferenceRenderingContext(), and the surface would be gfxPlatform::ScreenReferenceSurface(). When is the surface you get not gfxPlatform::ScreenReferenceSurface()?
(In reply to comment #22)
> But in all cases I would have thought the context involved would be the result
> of PresShell::GetReferenceRenderingContext(), and the surface would be
> gfxPlatform::ScreenReferenceSurface().

This is true for the initial construction of the "Minefield" textrun. In this case, the surface is a D2D surface.

> When is the surface you get not
> gfxPlatform::ScreenReferenceSurface()?

Here's a stack for the case where the textrun is being rebuild during layer painting, and a context with an image surface has apparently been passed down from the layers code:

>xul.dll!gfxFont::InitTextRun(gfxContext * aContext=0x0fca4a98, gfxTextRun * aTextRun=0x0fc9b048, const wchar_t * aString=0x0030a7c4, unsigned int aRunStart=0, unsigned int aRunLength=10, int aRunScript=25, int aPreferPlatformShaping=0)  Line 1449C++
 xul.dll!gfxFontGroup::InitTextRun(gfxContext * aContext=0x0fca4a98, gfxTextRun * aTextRun=0x0fc9b048, const wchar_t * aString=0x0030a7c4, unsigned int aTotalLength=10, unsigned int aScriptRunStart=0, unsigned int aScriptRunEnd=10, int aRunScript=25)  Line 2427 + 0x27 bytesC++
 xul.dll!gfxFontGroup::InitTextRun(gfxContext * aContext=0x0fca4a98, gfxTextRun * aTextRun=0x0fc9b048, const wchar_t * aString=0x0030a7c4, unsigned int aLength=10)  Line 2391C++
 xul.dll!gfxFontGroup::MakeTextRun(const wchar_t * aString=0x0030a7c4, unsigned int aLength=10, const gfxTextRunFactory::Parameters * aParams=0x0030ad50, unsigned int aFlags=1)  Line 2367C++
 xul.dll!TextRunWordCache::MakeTextRun(const wchar_t * aText=0x094f82b8, unsigned int aLength=9, gfxFontGroup * aFontGroup=0x093c9ec0, const gfxTextRunFactory::Parameters * aParams=0x0030adac, unsigned int aFlags=0)  Line 726 + 0x30 bytesC++
 xul.dll!gfxTextRunWordCache::MakeTextRun(const wchar_t * aText=0x094f82b8, unsigned int aLength=9, gfxFontGroup * aFontGroup=0x093c9ec0, const gfxTextRunFactory::Parameters * aParams=0x0030adac, unsigned int aFlags=0)  Line 1044C++
 xul.dll!gfxTextRunCache::MakeTextRun(const wchar_t * aText=0x094f82b8, unsigned int aLength=9, gfxFontGroup * aFontGroup=0x093c9ec0, gfxContext * aRefContext=0x0fca4a98, unsigned int aAppUnitsPerDevUnit=60, unsigned int aFlags=0)  Line 93 + 0x19 bytesC++
 xul.dll!nsThebesFontMetrics::AutoTextRun::AutoTextRun(nsThebesFontMetrics * aMetrics=0x0af2ea78, nsIRenderingContext * aRC=0x0fcd8480, const wchar_t * aString=0x094f82b8, int aLength=9)  Line 171 + 0x3e bytesC++
 xul.dll!nsThebesFontMetrics::DrawString(const wchar_t * aString=0x094f82b8, unsigned int aLength=9, int aX=1200, int aY=900, int aFontID=-1, const int * aSpacing=0x00000000, nsThebesRenderingContext * aContext=0x0fcd8480)  Line 419C++
 xul.dll!nsThebesRenderingContext::DrawStringInternal(const wchar_t * aString=0x094f82b8, unsigned int aLength=9, int aX=1200, int aY=900, int aFontID=-1, const int * aSpacing=0x00000000)  Line 1388C++
 xul.dll!nsThebesRenderingContext::DrawString(const wchar_t * aString=0x094f82b8, unsigned int aLength=9, int aX=1200, int aY=900, int aFontID=-1, const int * aSpacing=0x00000000)  Line 1210 + 0x20 bytesC++
 xul.dll!nsThebesRenderingContext::DrawString(const nsString & aString={...}, int aX=1200, int aY=900, int aFontID=-1, const int * aSpacing=0x00000000)  Line 878C++
 xul.dll!nsTextBoxFrame::DrawText(nsIRenderingContext & aRenderingContext={...}, const nsRect & aTextRect={...}, const unsigned int * aOverrideColor=0x0030b030)  Line 554C++
 xul.dll!nsTextBoxFrame::PaintOneShadow(gfxContext * aCtx=0x0aea1480, const nsRect & aTextRect={...}, nsCSSShadowItem * aShadowDetails=0x0fca5874, const unsigned int & aForegroundColor=4294967295, const nsRect & aDirtyRect={...})  Line 616C++
 xul.dll!nsTextBoxFrame::PaintTitle(nsIRenderingContext & aRenderingContext={...}, const nsRect & aDirtyRect={...}, nsPoint aPt={...})  Line 402C++
 xul.dll!nsDisplayXULTextBox::Paint(nsDisplayListBuilder * aBuilder=0x0030c338, nsIRenderingContext * aCtx=0x0fca5c38)  Line 359C++
 xul.dll!mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer * aLayer=0x0b3ec170, gfxContext * aContext=0x0aea1480, const nsIntRegion & aRegionToDraw={...}, const nsIntRegion & aRegionToInvalidate={...}, void * aCallbackData=0x0030c338)  Line 1697C++
 xul.dll!mozilla::layers::BasicThebesLayer::Paint(gfxContext * aContext=0x0aea1480, void (mozilla::layers::ThebesLayer *, gfxContext *, const nsIntRegion &, const nsIntRegion &, void *)* aCallback=0x5ed0e100, void * aCallbackData=0x0030c338)  Line 484 + 0x28 bytesC++
 xul.dll!mozilla::layers::BasicLayerManager::PaintLayer(mozilla::layers::Layer * aLayer=0x0b3ec170, void (mozilla::layers::ThebesLayer *, gfxContext *, const nsIntRegion &, const nsIntRegion &, void *)* aCallback=0x5ed0e100, void * aCallbackData=0x0030c338)  Line 1287C++
 xul.dll!mozilla::layers::BasicLayerManager::PaintLayer(mozilla::layers::Layer * aLayer=0x0b3ebfb0, void (mozilla::layers::ThebesLayer *, gfxContext *, const nsIntRegion &, const nsIntRegion &, void *)* aCallback=0x5ed0e100, void * aCallbackData=0x0030c338)  Line 1290C++
 xul.dll!mozilla::layers::BasicLayerManager::EndTransaction(void (mozilla::layers::ThebesLayer *, gfxContext *, const nsIntRegion &, const nsIntRegion &, void *)* aCallback=0x5ed0e100, void * aCallbackData=0x0030c338)  Line 1204C++
 xul.dll!mozilla::layers::BasicShadowLayerManager::EndTransaction(void (mozilla::layers::ThebesLayer *, gfxContext *, const nsIntRegion &, const nsIntRegion &, void *)* aCallback=0x5ed0e100, void * aCallbackData=0x0030c338)  Line 2514C++
 xul.dll!nsDisplayList::PaintForFrame(nsDisplayListBuilder * aBuilder=0x0030c338, nsIRenderingContext * aCtx=0x00000000, nsIFrame * aForFrame=0x09485a20, unsigned int aFlags=1)  Line 478C++
 xul.dll!nsDisplayList::PaintRoot(nsDisplayListBuilder * aBuilder=0x0030c338, nsIRenderingContext * aCtx=0x00000000, unsigned int aFlags=1)  Line 385C++
 xul.dll!nsLayoutUtils::PaintFrame(nsIRenderingContext * aRenderingContext=0x00000000, nsIFrame * aFrame=0x09485a20, const nsRegion & aDirtyRegion={...}, unsigned int aBackstop=0, unsigned int aFlags=4)  Line 1436C++
 xul.dll!PresShell::Paint(nsIView * aDisplayRoot=0x09483538, nsIView * aViewToPaint=0x09483538, nsIWidget * aWidgetToPaint=0x093a3548, const nsRegion & aDirtyRegion={...}, const nsIntRegion & aIntDirtyRegion={...}, int aPaintDefaultBackground=0, int aWillSendDidPaint=0)  Line 6095 + 0x15 bytesC++
 xul.dll!nsViewManager::RenderViews(nsView * aView=0x09483538, nsIWidget * aWidget=0x093a3548, const nsRegion & aRegion={...}, const nsIntRegion & aIntRegion={...}, int aPaintDefaultBackground=0, int aWillSendDidPaint=0)  Line 448C++
 xul.dll!nsViewManager::Refresh(nsView * aView=0x09483538, nsIWidget * aWidget=0x093a3548, const nsIntRegion & aRegion={...}, unsigned int aUpdateFlags=1)  Line 415C++
 xul.dll!nsViewManager::DispatchEvent(nsGUIEvent * aEvent=0x0030ca7c, nsIView * aView=0x09483538, nsEventStatus * aStatus=0x0030c874)  Line 914C++
 xul.dll!AttachedHandleEvent(nsGUIEvent * aEvent=0x0030ca7c)  Line 194C++
 xul.dll!nsWindow::DispatchEvent(nsGUIEvent * event=0x0030ca7c, nsEventStatus & aStatus=nsEventStatus_eIgnore)  Line 3617 + 0xc bytesC++
 xul.dll!nsWindow::DispatchWindowEvent(nsGUIEvent * event=0x0030ca7c, nsEventStatus & aStatus=nsEventStatus_eIgnore)  Line 3649C++
 xul.dll!nsWindow::OnPaint(HDC__ * aDC=0x00000000, unsigned int aNestingLevel=0)  Line 483 + 0x24 bytesC++
 xul.dll!nsWindow::ProcessMessage(unsigned int msg=15, unsigned int & wParam=0, long & lParam=0, long * aRetValue=0x0030d29c)  Line 4868 + 0xf bytesC++
 xul.dll!nsWindow::WindowProcInternal(HWND__ * hWnd=0x000f0506, unsigned int msg=15, unsigned int wParam=0, long lParam=0)  Line 4457 + 0x20 bytesC++
 xul.dll!nsWindow::WindowProc(HWND__ * hWnd=0x000f0506, unsigned int msg=15, unsigned int wParam=0, long lParam=0)  Line 4409 + 0x15 bytesC++
Oh, nsTextBoxFrame, of course.

The problem is that text for shadows is painted to an image surface so we can get the alpha channel and blur it. And nsTextBoxFrame goes through nsIRenderingContext::DrawString which creates the textrun and paints it in one operation. It needs to work like nsTextFrame instead: call MakeTextRun directly to make the textrun (passing GetReferenceRenderingContext), then render the textrun to the destination context using gfxTextRun::Draw.
Not only the Firefox button changes, but the tab header text as well! Just look at the 2nd tab of the attachment called screenshot: https://bugzilla.mozilla.org/attachment.cgi?id=492067

I have no repro, but I have seen this a few times.
Assignee: nobody → roc
Attached patch fix — — Splinter Review 
I can't reproduce this, but this should fix it.
Attachment #502679 - Flags: review?(dbaron)
Comment on attachment 502679 [details] [diff] [review]
fix

Jonathan, I guess you can tell me if this fixes it.
Attachment #502679 - Flags: feedback?(jfkthame)
Whiteboard: [hardblocker](?) → [hardblocker](?)[needs review]
I built the trunk and reproduced the bug with lots of Win+D and Win+Up/Down (the AutoIt script did not work). Then applied the patch and I could not reproduce it. So this seems to fix it, but I will spend more time looking at it.

Does the fix render the button the same as before pixel by pixel or should I expect differences?
Does it fix the tab header text as well?
It should fix them all. Thanks.

The text shadows may be rendered slightly differently. That should be OK.
Comment on attachment 502679 [details] [diff] [review]
fix

(We should really clean up the unneeded methods and parameters here sometime.)

>-    mCtx->DrawString(mText, mLength, mPt.x + aXOffset, mPt.y);
>+    nsCOMPtr<nsIFontMetrics> metrics;
>+    mCtx->GetFontMetrics(*getter_AddRefs(metrics));
>+    nsIThebesFontMetrics* fm = static_cast<nsIThebesFontMetrics*>(metrics.get());
>+    fm->DrawString(mText, mLength, mPt.x + aXOffset, mPt.y,
>+                   mCtx, mTextRunConstructionContext);

This means you're skipping the safe-length code in the rendering context.
That seems bad.  Or is it ok somehow?

>-       aRenderingContext.DrawString(mCroppedTitle, aTextRect.x, baseline);
>+       nsIThebesFontMetrics* fm = static_cast<nsIThebesFontMetrics*>(fontMet.get());
>+       fm->DrawString(mCroppedTitle.get(), mCroppedTitle.Length(),
>+                      aTextRect.x, baseline, &aRenderingContext, refContext.get());

Same here.

A fix for these might lead to wanting the new DrawString method wanting to be consistent with the others and take nsThebesRenderingContext. 


Otherwise this looks fine.
(In reply to comment #31)
> Comment on attachment 502679 [details] [diff] [review]
> fix
> 
> (We should really clean up the unneeded methods and parameters here sometime.)
> 
> >-    mCtx->DrawString(mText, mLength, mPt.x + aXOffset, mPt.y);
> >+    nsCOMPtr<nsIFontMetrics> metrics;
> >+    mCtx->GetFontMetrics(*getter_AddRefs(metrics));
> >+    nsIThebesFontMetrics* fm = static_cast<nsIThebesFontMetrics*>(metrics.get());
> >+    fm->DrawString(mText, mLength, mPt.x + aXOffset, mPt.y,
> >+                   mCtx, mTextRunConstructionContext);
> 
> This means you're skipping the safe-length code in the rendering context.
> That seems bad.  Or is it ok somehow?

I think that code is no longer needed; gfxTextRuns can be arbitrary length. We now have code in the MakeTextRun implementations that automatically splits up text for processing.
Comment on attachment 502679 [details] [diff] [review]
fix

ok, r=dbaron.

But at some point we should (a) clean up the unneeded splitting code and (b) clean up the unneeded/unused parameters to and variants of the DrawString methods.  It might be worth filing bugs on this.
Attachment #502679 - Flags: review?(dbaron) → review+
We really should just get rid of all the nsIRenderingContext string code. Instead we can use nsLayoutUtils methods that operate on textruns and gfxContext directly.
Whiteboard: [hardblocker](?)[needs review] → [hardblocker](?)[needs landing]
http://hg.mozilla.org/mozilla-central/rev/6c727b25207a
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [hardblocker](?)[needs landing] → [hardblocker](?)
I can no longer reproduce it in the daily builds. Nice job!

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10pre) Gecko/20110118 Firefox/4.0b10pre
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: