Bug 36853 – REGRESSION(r56552): Broken scrollbars size

• Status: CLOSED
• Resolution: FIXED
• Component: New Bugs

[Kenneth Rohde Christiansen]

When there is only scroll in one direction the scroll arrows are not at the right position

[Kenneth Rohde Christiansen]

Created attachment 52083 [details]
Screenshot 2

[Kenneth Rohde Christiansen]

Created attachment 52084 [details]
Screenshot 1

[Csaba Osztrogonác]

Expected file and pixel result should be updated after fix in https://bugs.webkit.org/show_bug.cgi?id=36058

[Antonio Gomes]

regression from bug 21300, r56552

[Antonio Gomes]

Created attachment 52991 [details]
patch v1

attempt to get feedback

[Kenneth Rohde Christiansen]

(In reply to comment #5)
> Created an attachment (id=52991) [details]
> patch v1
> 
> attempt to get feedback

What do you mean with attempt? Are you not sure it is fixing the issue? What is the status of this patch?

[Kenneth Rohde Christiansen]

Comment on attachment 52991 [details]
patch v1


> +    int scollbarThickness =
> +#if defined(Q_WS_MAC) && !defined(QT_MAC_USE_COCOA)
> +        ScrollbarTheme::nativeTheme()->scrollbarThickness();
> +#else
> +        0;
> +#endif

I think you should include the 'int scollbarThickness =' inside the ifdef's

The issue that bug 21300 (the regression origin) was working around was a Mac Carbon
 9         fix for the "Grow Box" at the bottom right side of the application window: it was using
 10         a rect sizing scrollbar thickness as the dimensions of this "Grow Box" area.

I would say:

The regression was caused by r56552, which introduced a fix to bug webkit.org/b/21300. 
The bug solved an issue with the resize handle on mac, but did it in a way that 
affected all Qt platforms and thus broke the behavior on non-mac platforms.

This patch makes the mac specific change ifdef'ed and only applied for the mac platform.

[Antonio Gomes]

(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=52991) [details] [details]
> > patch v1
> > 
> > attempt to get feedback
> 
> What do you mean with attempt? Are you not sure it is fixing the issue? What is
> the status of this patch?

The intention was exactly what you got me in comment #7. thank you, I've changed the ChangeLog content for the better now.

[Antonio Gomes]

Created attachment 53137 [details]
(committed with r57656) patch v2

Differently from patch 52991 (v1), patch v2 makes the whole method body mac cocoa specific. For other platforms it just returns an empty rect as before (similarly on gtk, win, wx, efl and haiku ports).

Thank you for the feedback Kenneth.

Tor Arne, could you also validate that?

[Antonio Gomes]

Ping review! :)

[Kenneth Rohde Christiansen]

Comment on attachment 53137 [details]
(committed with r57656) patch v2

LGTM, r=me.

[Tor Arne Vestbø]

Comment on attachment 53137 [details]
(committed with r57656) patch v2


> +#if defined(Q_WS_MAC) && !defined(QT_MAC_USE_COCOA)

This is not carbon-specific, you should remove the last part of the ifdef.

[Antonio Gomes]

Comment on attachment 53137 [details]
(committed with r57656) patch v2

Clearing flags on attachment: 53137

Committed r57656: <http://trac.webkit.org/changeset/57656>

[Antonio Gomes]

(In reply to comment #12)
> (From update of attachment 53137 [details])
> 
> > +#if defined(Q_WS_MAC) && !defined(QT_MAC_USE_COCOA)
> 
> This is not carbon-specific, you should remove the last part of the ifdef.

Fixed before committing!

Thank you Tor Arne and Kenneth.

[Simon Hausmann]

Revision r57656 cherry-picked into qtwebkit-2.0 with commit c960094253dade2b29e368bf16e48bc9a61da6f6