RESOLVED FIXED 104219
Page popup should align to the right when the anchor element is rtl
https://bugs.webkit.org/show_bug.cgi?id=104219
Summary Page popup should align to the right when the anchor element is rtl
Keishi Hattori
Reported 2012-12-05 22:34:26 PST
Page popup should align to the right when the anchor element is rtl
Attachments
Patch (2.29 KB, patch)
2012-12-05 23:13 PST, Keishi Hattori
no flags
Patch (3.13 KB, patch)
2012-12-05 23:50 PST, Keishi Hattori
no flags
Patch (3.29 KB, patch)
2012-12-06 00:15 PST, Keishi Hattori
no flags
Patch (3.29 KB, patch)
2012-12-06 00:34 PST, Keishi Hattori
no flags
Keishi Hattori
Comment 1 2012-12-05 23:13:06 PST
Kent Tamura
Comment 2 2012-12-05 23:17:53 PST
Comment on attachment 177948 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177948&action=review > Source/WebCore/Resources/pagepopups/pickerCommon.js:173 > - resizeWindow(1, 1); > + setWindowRect(new Rectangle(window.screen.left, window.screen.top, 1, 1)); I don't understand this. window.screen is a Screen object, and it has not left and top properties.
Keishi Hattori
Comment 3 2012-12-05 23:50:15 PST
Keishi Hattori
Comment 4 2012-12-05 23:53:27 PST
(In reply to comment #2) > (From update of attachment 177948 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177948&action=review > > > Source/WebCore/Resources/pagepopups/pickerCommon.js:173 > > - resizeWindow(1, 1); > > + setWindowRect(new Rectangle(window.screen.left, window.screen.top, 1, 1)); > > I don't understand this. window.screen is a Screen object, and it has not left and top properties. I made a mistake.
Kent Tamura
Comment 5 2012-12-06 00:01:51 PST
Comment on attachment 177954 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177954&action=review > Source/WebCore/ChangeLog:15 > + (setWindowRect): If the window is hidden we want to move first then resize so the popup doesn't flicker. > + (isWindowHidden): Returns true if the window is hidden using hideWindow(). nit: These changes look unrelated to the RTL issue. > Source/WebCore/Resources/pagepopups/pickerCommon.js:172 > + var isHidden = isWindowHidden(); > + if (!isHidden) > + window.resizeTo(rect.width, rect.height); > window.moveTo(rect.x - window.screen.availLeft, rect.y - window.screen.availTop); > - window.resizeTo(rect.width, rect.height); > + if (isHidden) > + window.resizeTo(rect.width, rect.height); nit: it's ok to write duplicated line to improve readability. if (isWindowHidden()) { window.moveTo(...); window.resizeTo(...); } else { window.resizeTo(...); window.moveTo(...); }
Keishi Hattori
Comment 6 2012-12-06 00:06:25 PST
(In reply to comment #5) > (From update of attachment 177954 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177954&action=review > > > Source/WebCore/ChangeLog:15 > > + (setWindowRect): If the window is hidden we want to move first then resize so the popup doesn't flicker. > > + (isWindowHidden): Returns true if the window is hidden using hideWindow(). > > nit: These changes look unrelated to the RTL issue. When the element is rtl and we call hideWindow, I see the popup move to the right edge first and then shrink to 1x1.
Keishi Hattori
Comment 7 2012-12-06 00:15:31 PST
WebKit Review Bot
Comment 8 2012-12-06 00:18:45 PST
Comment on attachment 177959 [details] Patch Rejecting attachment 177959 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/15158631
Keishi Hattori
Comment 9 2012-12-06 00:34:21 PST
WebKit Review Bot
Comment 10 2012-12-06 01:04:23 PST
Comment on attachment 177962 [details] Patch Clearing flags on attachment: 177962 Committed r136821: <http://trac.webkit.org/changeset/136821>
WebKit Review Bot
Comment 11 2012-12-06 01:04:28 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.