Bug 66062 – <select> popup menu appears in wrong place when page is zoomed

• Component: New Bugs
• Status: NEW

[Fady Samuel]

Platform PopupMenu Code Should Have Access To transformed bounding box of RenderMenuList

[Fady Samuel]

Created attachment 103628 [details]
Patch

[Fady Samuel]

(In reply to comment #0)
> Platform PopupMenu Code Should Have Access To transformed bounding box of RenderMenuList

This touches a lot of platform-specific code. If there's a nicer way to pass this information along to platform code, then please offer suggestions. We may use this information in certain Chromium platforms.

[WebKit Review Bot]

Attachment 103628 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 135, in <module>
    main()
  File "Tools/Scripts/check-webkit-style", line 122, in main
    patch_checker.check(patch)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/patchreader.py", line 66, in check
    self._text_file_reader.process_file(file_path=path, line_numbers=line_numbers)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filereader.py", line 128, in process_file
    self._processor.process(lines, file_path, **kwargs)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 800, in process
    checker.check(lines)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 3556, in check
    self.handle_style_error, self.min_confidence)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 3432, in _process_lines
    include_state, function_state, class_state, file_state, error)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 3397, in process_line
    check_style(clean_lines, line, file_extension, class_state, file_state, error)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 2521, in check_style
    check_spacing(file_extension, clean_lines, line_number, error)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 1903, in check_spacing
    error(line_number, 'whitespace/braces', 5, 'Missing space inside { }.')
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/error_handlers.py", line 145, in __call__
    file_path=self._file_path):
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 643, in is_reportable
    return self._filter_configuration.should_check(category, file_path)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filter.py", line 277, in should_check
    return self._filter_from_path(path).should_check(category)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filter.py", line 240, in _filter_from_path
    path_rules = self._path_rules_from_path(path)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filter.py", line 212, in _path_rules_from_path
    for (sub_paths, path_rules) in self._get_path_specific_lower():
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filter.py", line 200, in _get_path_specific_lower
    sub_paths = map(str.lower, sub_paths)
TypeError: descriptor 'lower' requires a 'str' object but received a 'tuple'


If any of these errors are false positives, please file a bug against check-webkit-style.

[Fady Samuel]

(In reply to comment #3)
> Attachment 103628 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> 
> Traceback (most recent call last):
>   File "Tools/Scripts/check-webkit-style", line 135, in <module>
>     main()
>   File "Tools/Scripts/check-webkit-style", line 122, in main
>     patch_checker.check(patch)
>   File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/patchreader.py", line 66, in check
>     self._text_file_reader.process_file(file_path=path, line_numbers=line_numbers)
>   File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filereader.py", line 128, in process_file
>     self._processor.process(lines, file_path, **kwargs)
>   File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 800, in process
>     checker.check(lines)
>   File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 3556, in check
>     self.handle_style_error, self.min_confidence)
>   File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 3432, in _process_lines
>     include_state, function_state, class_state, file_state, error)
>   File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 3397, in process_line
>     check_style(clean_lines, line, file_extension, class_state, file_state, error)
>   File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 2521, in check_style
>     check_spacing(file_extension, clean_lines, line_number, error)
>   File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 1903, in check_spacing
>     error(line_number, 'whitespace/braces', 5, 'Missing space inside { }.')
>   File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/error_handlers.py", line 145, in __call__
>     file_path=self._file_path):
>   File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 643, in is_reportable
>     return self._filter_configuration.should_check(category, file_path)
>   File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filter.py", line 277, in should_check
>     return self._filter_from_path(path).should_check(category)
>   File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filter.py", line 240, in _filter_from_path
>     path_rules = self._path_rules_from_path(path)
>   File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filter.py", line 212, in _path_rules_from_path
>     for (sub_paths, path_rules) in self._get_path_specific_lower():
>   File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filter.py", line 200, in _get_path_specific_lower
>     sub_paths = map(str.lower, sub_paths)
> TypeError: descriptor 'lower' requires a 'str' object but received a 'tuple'
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

This looks like a broken script :(

[WebKit Review Bot]

Comment on attachment 103628 [details]
Patch

Attachment 103628 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9357247

[Fady Samuel]

Created attachment 103632 [details]
Patch

[WebKit Review Bot]

Attachment 103632 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 135, in <module>
    main()
  File "Tools/Scripts/check-webkit-style", line 122, in main
    patch_checker.check(patch)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/patchreader.py", line 66, in check
    self._text_file_reader.process_file(file_path=path, line_numbers=line_numbers)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filereader.py", line 128, in process_file
    self._processor.process(lines, file_path, **kwargs)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 800, in process
    checker.check(lines)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 3556, in check
    self.handle_style_error, self.min_confidence)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 3432, in _process_lines
    include_state, function_state, class_state, file_state, error)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 3397, in process_line
    check_style(clean_lines, line, file_extension, class_state, file_state, error)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 2521, in check_style
    check_spacing(file_extension, clean_lines, line_number, error)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 1903, in check_spacing
    error(line_number, 'whitespace/braces', 5, 'Missing space inside { }.')
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/error_handlers.py", line 145, in __call__
    file_path=self._file_path):
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 643, in is_reportable
    return self._filter_configuration.should_check(category, file_path)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filter.py", line 277, in should_check
    return self._filter_from_path(path).should_check(category)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filter.py", line 240, in _filter_from_path
    path_rules = self._path_rules_from_path(path)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filter.py", line 212, in _path_rules_from_path
    for (sub_paths, path_rules) in self._get_path_specific_lower():
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filter.py", line 200, in _get_path_specific_lower
    sub_paths = map(str.lower, sub_paths)
TypeError: descriptor 'lower' requires a 'str' object but received a 'tuple'


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 103632 [details]
Patch

Attachment 103632 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9371064

[Darin Fisher (:fishd, Google)]

Comment on attachment 103632 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103632&action=review

> Source/WebCore/ChangeLog:3
> +        Platform PopupMenu Code Should Have Access To transformed bounding box of RenderMenuList

i don't really understand what a "transformed bounding box of RenderMenuList" is.  can you explain that to me?

> Source/WebCore/platform/PopupMenu.h:34
> +    virtual void show(const IntRect&, const IntRect&, FrameView*, int index) = 0;

It is extremely non-obvious what these two IntRect parameters mean.  When it is not obvious, they should have a parameter name.

[Fady Samuel]

(In reply to comment #9)
> (From update of attachment 103632 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103632&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Platform PopupMenu Code Should Have Access To transformed bounding box of RenderMenuList
> 
> i don't really understand what a "transformed bounding box of RenderMenuList" is.  can you explain that to me?
> 
> > Source/WebCore/platform/PopupMenu.h:34
> > +    virtual void show(const IntRect&, const IntRect&, FrameView*, int index) = 0;
> 
> It is extremely non-obvious what these two IntRect parameters mean.  When it is not obvious, they should have a parameter name.

Let me be very specific about the desired functionality here:

Ultimately we want the page scale factor to be taken into account when positioning popups on certain Chromium platforms. To do that, the chromium platform layer needs to have access to the size of the combobox after the application of the scaling (and other) transforms.

[Fady Samuel]

Created attachment 104109 [details]
Patch

[WebKit Review Bot]

Attachment 104109 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/gtk/PopupMenuGtk.h:38:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/PopupMenu.h:34:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/loader/EmptyClients.h:72:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebKit2/WebProcess/WebCoreSupport/WebPopupMenu.h:52:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/ExternalPopupMenu.h:59:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/mac/PopupMenuMac.h:46:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/efl/PopupMenuEfl.h:39:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/brew/PopupMenuBrew.h:39:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/haiku/PopupMenuHaiku.h:41:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/wx/PopupMenuWx.h:48:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/chromium/PopupMenuChromium.h:52:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 11 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Comment on attachment 104109 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104109&action=review

>> Source/WebCore/platform/PopupMenu.h:34
>> +    virtual void show(const IntRect& rect, const IntRect& transformedRect, FrameView*, int index) = 0;
> 
> The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]

The style bot is right. "rect" and "transformedRect" aren't really specific enough. It would be good for the names to indicate what coordinate space the rects are in.

> Source/WebCore/rendering/RenderTextControlSingleLine.cpp:185
> -    m_searchPopup->popupMenu()->show(absoluteBoundingBoxRect(true), document()->view(), -1);
> +    IntRect boundingBox = absoluteBoundingBoxRect(true);
> +    m_searchPopup->popupMenu()->show(boundingBox, boundingBox, document()->view(), -1);

Why are you passing the same rect twice in this case?

[Fady Samuel]

(In reply to comment #13)
> (From update of attachment 104109 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104109&action=review
> 
> >> Source/WebCore/platform/PopupMenu.h:34
> >> +    virtual void show(const IntRect& rect, const IntRect& transformedRect, FrameView*, int index) = 0;
> > 
> > The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
> 
> The style bot is right. "rect" and "transformedRect" aren't really specific enough. It would be good for the names to indicate what coordinate space the rects are in.
> 
> > Source/WebCore/rendering/RenderTextControlSingleLine.cpp:185
> > -    m_searchPopup->popupMenu()->show(absoluteBoundingBoxRect(true), document()->view(), -1);
> > +    IntRect boundingBox = absoluteBoundingBoxRect(true);
> > +    m_searchPopup->popupMenu()->show(boundingBox, boundingBox, document()->view(), -1);
> 
> Why are you passing the same rect twice in this case?

I will fix the names now. As for RenderTextControlSingleLine...it already expected taking in the transformed rect..I am looking into making this more consistent now.

[Fady Samuel]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 104109 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=104109&action=review
> > 
> > >> Source/WebCore/platform/PopupMenu.h:34
> > >> +    virtual void show(const IntRect& rect, const IntRect& transformedRect, FrameView*, int index) = 0;
> > > 
> > > The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
> > 
> > The style bot is right. "rect" and "transformedRect" aren't really specific enough. It would be good for the names to indicate what coordinate space the rects are in.
> > 
> > > Source/WebCore/rendering/RenderTextControlSingleLine.cpp:185
> > > -    m_searchPopup->popupMenu()->show(absoluteBoundingBoxRect(true), document()->view(), -1);
> > > +    IntRect boundingBox = absoluteBoundingBoxRect(true);
> > > +    m_searchPopup->popupMenu()->show(boundingBox, boundingBox, document()->view(), -1);
> > 
> > Why are you passing the same rect twice in this case?
> 
> I will fix the names now. As for RenderTextControlSingleLine...it already expected taking in the transformed rect..I am looking into making this more consistent now.

Are localRect, and absoluteRect sufficiently descriptive parameter names?

[Fady Samuel]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (From update of attachment 104109 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=104109&action=review
> > > 
> > > >> Source/WebCore/platform/PopupMenu.h:34
> > > >> +    virtual void show(const IntRect& rect, const IntRect& transformedRect, FrameView*, int index) = 0;
> > > > 
> > > > The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
> > > 
> > > The style bot is right. "rect" and "transformedRect" aren't really specific enough. It would be good for the names to indicate what coordinate space the rects are in.
> > > 
> > > > Source/WebCore/rendering/RenderTextControlSingleLine.cpp:185
> > > > -    m_searchPopup->popupMenu()->show(absoluteBoundingBoxRect(true), document()->view(), -1);
> > > > +    IntRect boundingBox = absoluteBoundingBoxRect(true);
> > > > +    m_searchPopup->popupMenu()->show(boundingBox, boundingBox, document()->view(), -1);
> > > 
> > > Why are you passing the same rect twice in this case?
> > 
> > I will fix the names now. As for RenderTextControlSingleLine...it already expected taking in the transformed rect..I am looking into making this more consistent now.
> 
> Are localRect, and absoluteRect sufficiently descriptive parameter names?

Let me step back and ask another question, is there any reason why any platform would not want the transformed rect of the combo box?

Let me be specific about why we want this. We want a combo box popup to appear below a combo box even if the page is scaled up/down. Thus, we need to have the scaled height of the box.

[Fady Samuel]

Created attachment 106644 [details]
Popup mispositioned when page scaled up (should be underneath select element)

[Fady Samuel]

Created attachment 106650 [details]
Patch

[Fady Samuel]

Created attachment 106792 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 106792 [details]
Patch

Seems like we need to scale the font size, too, and any other values the PopupMenu gets from its client.

[Fady Samuel]

(In reply to comment #20)
> (From update of attachment 106792 [details])
> Seems like we need to scale the font size, too, and any other values the PopupMenu gets from its client.

Font size is already scaled. I believe PopupMenuStyles are all scaled already.

[Adam Roben (:aroben)]

Comment on attachment 106792 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106792&action=review

> Source/WebCore/ChangeLog:4
> +        Platform code should take in the transformed rect of the select element
> +        https://bugs.webkit.org/show_bug.cgi?id=66062

Can you please retitle the bug to describe a user-visible symptom?

> Source/WebCore/ChangeLog:11
> +        (WebCore::RenderMenuList::showPopup):

This change needs some explanation. Just adding "true" at the callsite provides no meaning for reviewers or other folks who look back at this change later.

Why don't we have to change a function like showPopup for the Recent Searches menu?

> Source/WebCore/ChangeLog:16
> +        (WebCore::RenderMenuList::clientPaddingLeft):
> +        (WebCore::RenderMenuList::clientPaddingRight):
> +        * rendering/RenderTextControlSingleLine.cpp:
> +        (WebCore::RenderTextControlSingleLine::clientPaddingLeft):
> +        (WebCore::RenderTextControlSingleLine::clientPaddingRight):

These changes should also have some comments.

[Fady Samuel]

Comment on attachment 106792 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106792&action=review

>> Source/WebCore/ChangeLog:4
>> +        https://bugs.webkit.org/show_bug.cgi?id=66062
> 
> Can you please retitle the bug to describe a user-visible symptom?

Done

>> Source/WebCore/ChangeLog:11
>> +        (WebCore::RenderMenuList::showPopup):
> 
> This change needs some explanation. Just adding "true" at the callsite provides no meaning for reviewers or other folks who look back at this change later.
> 
> Why don't we have to change a function like showPopup for the Recent Searches menu?

Done and it already does apparently.

>> Source/WebCore/ChangeLog:16
>> +        (WebCore::RenderTextControlSingleLine::clientPaddingRight):
> 
> These changes should also have some comments.

Done.

[Fady Samuel]

Created attachment 106805 [details]
Patch

[Adam Roben (:aroben)]

(In reply to comment #23)
> (From update of attachment 106792 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106792&action=review
> 
> >> Source/WebCore/ChangeLog:4
> >> +        https://bugs.webkit.org/show_bug.cgi?id=66062
> > 
> > Can you please retitle the bug to describe a user-visible symptom?
> 
> Done

The current title still isn't a user-visible symptom. A user-visible symptom would be something like "<select> popup menu appears in wrong place when page is zoomed". That's something an actual user can witness.

[Adam Roben (:aroben)]

Comment on attachment 106805 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106805&action=review

> Source/WebCore/rendering/RenderMenuList.cpp:306
>      // Compute the top left taking transforms into account, but use
>      // the actual width of the element to size the popup.
>      FloatPoint absTopLeft = localToAbsolute(FloatPoint(), false, true);
> -    LayoutRect absBounds = absoluteBoundingBoxRect();
> +    // Compute the bounds taking transforms into account.
> +    LayoutRect absBounds = absoluteBoundingBoxRect(true);

The comment above yours seems to say that we were intentionally not passing "true" here. Do you know why that decision was made originally? You need to update that comment so it agrees with the current code.

> Source/WebCore/rendering/RenderMenuList.cpp:516
> +    // The padding in the popup box should scale with the pageScaleFactor.
> +    return leftPadding * frame()->pageScaleFactor();

This comment is just restating what the code says. We typically try to avoid those kinds of comments, as they don't add any information but increase the maintenance burden of the code. It's also very easy for them to become inaccurate as the code changes. I don't think you need a comment in these functions. But I do think it would be good to put explanations in the ChangeLog of your changes. Here's a good example ChangeLog entry: <http://trac.webkit.org/changeset/92556/trunk/Source/WebCore/ChangeLog>.

Ditto for all the other client functions.

[Adam Roben (:aroben)]

(In reply to comment #23)
> (From update of attachment 106792 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106792&action=review
>
> >> Source/WebCore/ChangeLog:11
> >> +        (WebCore::RenderMenuList::showPopup):
> > 
> > This change needs some explanation. Just adding "true" at the callsite provides no meaning for reviewers or other folks who look back at this change later.
> > 
> > Why don't we have to change a function like showPopup for the Recent Searches menu?
> 
> Done and it already does apparently.

You should mention in your ChangeLog that the Recent Searches menu already does this. Then reviewers won't think you forgot to change it.

[Fady Samuel]

Comment on attachment 106805 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106805&action=review

>> Source/WebCore/rendering/RenderMenuList.cpp:306
>> +    LayoutRect absBounds = absoluteBoundingBoxRect(true);
> 
> The comment above yours seems to say that we were intentionally not passing "true" here. Do you know why that decision was made originally? You need to update that comment so it agrees with the current code.

I'm not sure. It looks like Simon Fraser wrote that line for https://bugs.webkit.org/show_bug.cgi?id=15678. I'll ask him to take a look at this patch as well.

>> Source/WebCore/rendering/RenderMenuList.cpp:516
>> +    return leftPadding * frame()->pageScaleFactor();
> 
> This comment is just restating what the code says. We typically try to avoid those kinds of comments, as they don't add any information but increase the maintenance burden of the code. It's also very easy for them to become inaccurate as the code changes. I don't think you need a comment in these functions. But I do think it would be good to put explanations in the ChangeLog of your changes. Here's a good example ChangeLog entry: <http://trac.webkit.org/changeset/92556/trunk/Source/WebCore/ChangeLog>.
> 
> Ditto for all the other client functions.

Fixing this now, thanks.

[Simon Fraser (smfr)]

Comment on attachment 106805 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106805&action=review

>>> Source/WebCore/rendering/RenderMenuList.cpp:306
>>> +    LayoutRect absBounds = absoluteBoundingBoxRect(true);
>> 
>> The comment above yours seems to say that we were intentionally not passing "true" here. Do you know why that decision was made originally? You need to update that comment so it agrees with the current code.
> 
> I'm not sure. It looks like Simon Fraser wrote that line for https://bugs.webkit.org/show_bug.cgi?id=15678. I'll ask him to take a look at this patch as well.

It was probably to keep the popup a reasonable size if the transform was a rotation. I think the right approach is to use absoluteBoundingBoxRect(true), then set the width of the popup to the untransformed width of the renderer * pageScaleFactor.

Would be good to have some testcases here.

[Fady Samuel]

Created attachment 106881 [details]
Patch

[Darin Adler]

Comment on attachment 106881 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106881&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests because popups are very platform-dependent.

That is not a good reason to leave a bug fix untested.

[Darin Adler]

Comment on attachment 106881 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106881&action=review

>> Source/WebCore/ChangeLog:8
>> +        No new tests because popups are very platform-dependent.
> 
> That is not a good reason to leave a bug fix untested.

I agree it makes it harder to write a test, but we need tests for bug fixes.

[Fady Samuel]

(In reply to comment #32)
> (From update of attachment 106881 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106881&action=review
> 
> >> Source/WebCore/ChangeLog:8
> >> +        No new tests because popups are very platform-dependent.
> > 
> > That is not a good reason to leave a bug fix untested.
> 
> I agree it makes it harder to write a test, but we need tests for bug fixes.

Can popups be tested in a layout test [I haven't tried yet]?

[Fady Samuel]

(In reply to comment #33)
> (In reply to comment #32)
> > (From update of attachment 106881 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=106881&action=review
> > 
> > >> Source/WebCore/ChangeLog:8
> > >> +        No new tests because popups are very platform-dependent.
> > > 
> > > That is not a good reason to leave a bug fix untested.
> > 
> > I agree it makes it harder to write a test, but we need tests for bug fixes.
> 
> Can popups be tested in a layout test [I haven't tried yet]?

So I can't test popups in layout tests. I'd love to upstream this fix as this is a real issue on certain Chromium platforms but I just don't know how to produce an automated test for it. The current layout test infrastructure does not allow for this kind of test. Could someone more experienced than I please give me suggestions?

[Dimitri Glazkov (Google)]

I can see window.internals test for this -- it should just query coordinates, and see if we get the right ones. WDYT?

[Fady Samuel]

(In reply to comment #35)
> I can see window.internals test for this -- it should just query coordinates, and see if we get the right ones. WDYT?

I shall attempt this thanks.

[Eric Seidel (no email)]

Comment on attachment 106881 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106881&action=review

> Source/WebCore/rendering/RenderTextControlSingleLine.cpp:639
> -    return padding;
> +    return padding * frame()->pageScaleFactor();

Really?  I thought we applied the pageScaleFactor at only a very few high-level places?  Are you sure all these low-level rendering objects need to know about it?   If so, how do we find all the places it's needed?