Bug 36853

Summary:

REGRESSION(r56552): Broken scrollbars size

Product:

WebKit

Reporter:

Kenneth Rohde Christiansen <kenneth>

Component:

New Bugs

Assignee:

Antonio Gomes <tonikitoo>

Status:

CLOSED FIXED

Severity:

Major

CC:

hausmann, hyatt, ossy, tonikitoo, vestbo

Priority:

Keywords:

Version:

528+ (Nightly build)

Hardware:

OS:

Linux

Bug Depends on:

Bug Blocks:

35784, 36058

Attachments:

Description	Flags
Screenshot 2	none
Screenshot 1	none
patch v1	kenneth: review-, tonikitoo: commit-queue-
(committed with r57656) patch v2	none

Kenneth Rohde Christiansen

Reported 2010-03-30 14:22:16 PDT

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

Attachments
Screenshot 2 (2.22 KB, image/png) 2010-03-30 14:23 PDT, Kenneth Rohde Christiansen	no flags	Details
Screenshot 1 (2.02 KB, image/png) 2010-03-30 14:23 PDT, Kenneth Rohde Christiansen	no flags	Details
patch v1 (2.85 KB, patch) 2010-04-09 14:50 PDT, Antonio Gomes	kenneth: review- tonikitoo: commit-queue-	Details Formatted Diff Diff
(committed with r57656) patch v2 (2.74 KB, patch) 2010-04-11 19:53 PDT, Antonio Gomes	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Kenneth Rohde Christiansen

Comment 1 2010-03-30 14:23:03 PDT

Created attachment 52083 [details] Screenshot 2

Kenneth Rohde Christiansen

Comment 2 2010-03-30 14:23:21 PDT

Created attachment 52084 [details] Screenshot 1

Csaba Osztrogonác

Comment 3 2010-04-06 04:13:57 PDT

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

Antonio Gomes

Comment 4 2010-04-09 12:11:35 PDT

regression from bug 21300, r56552

Antonio Gomes

Comment 5 2010-04-09 14:50:24 PDT

Created attachment 52991 [details] patch v1 attempt to get feedback

Kenneth Rohde Christiansen

Comment 6 2010-04-09 14:53:30 PDT

(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 7 2010-04-09 14:58:50 PDT

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

Comment 8 2010-04-11 19:49:17 PDT

(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

Comment 9 2010-04-11 19:53:36 PDT

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

Comment 10 2010-04-13 05:47:41 PDT

Ping review! :)

Kenneth Rohde Christiansen

Comment 11 2010-04-14 13:29:05 PDT

Comment on attachment 53137 [details] (committed with r57656) patch v2 LGTM, r=me.

Tor Arne Vestbø

Comment 12 2010-04-15 10:55:59 PDT

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 13 2010-04-15 11:25:17 PDT

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

Comment 14 2010-04-15 11:27:03 PDT

(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

Comment 15 2010-04-20 13:07:21 PDT

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

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