`resize` property on generated content makes parent element resizable instead

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: lea, Assigned: jyc, Mentored)

Tracking

Trunk
mozilla50
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox50 fixed)

Details

()

Attachments

(2 attachments, 1 obsolete attachment)

Check testcase. Resize handle shows up correctly on the pseudo-element, but when it’s dragged, it’s the parent element that gets resized instead.

css3-ui states that the resize property applies to “elements with ‘overflow’ other than visible”, which includes pseudo-elements. http://dev.w3.org/csswg/css-ui/#resize
nsResizerFrame::GetContentToResize does:

  return parent ? parent->FindFirstNonChromeOnlyAccessContent() : nullptr;

This was done very explicitly in bug 442228 (see bug 442228 comment 9).  

Neil, do you recall why we needed to skip native anon content here?
Flags: needinfo?(enndeakin)
I believe that was to skip the anonymous editor div inside a <textarea>. The check could be changed to check for that explicitly is desired, although I don't know what that check would be.
Flags: needinfo?(enndeakin)
Could just explicitly check for an anonymous div whose parent is textarea...
Neil, did you want to do that?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(enndeakin)
Mentor: enndeakin
Flags: needinfo?(enndeakin)
Posted file testcase 1
Here's the testcase as an attachment (minor tweaks, e.g. dropped <script>), for archival purposes.

[I might assign this to a Layout intern in the next month or so, as a first bug.]
Assignee: nobody → jchan
OS: Mac OS X → All
Hardware: x86 → All
Version: 34 Branch → Trunk
(In reply to Lea Verou from comment #0)
> css3-ui states that the resize property applies to “elements with ‘overflow’
> other than visible”, which includes pseudo-elements.
> http://dev.w3.org/csswg/css-ui/#resize

The spec has this a bit later (which may have been added in the time since this bug was filed):
 # The effect of the resize property on generated content is undefined.
 # Implementations should not apply the resize property to generated content.

So I think Chrome's behavior here (the resizer widget having no effect) is correct, and we should switch to match that.

In other words, nsResizerFrame::GetContentToResize should return null, if |parent| is a ::before or ::after pseudo-element, probably.
...and actually, as Lea noted on the Chrome/WebKit bugs, it's a bit bogus that anyone's even *displaying* a resizer, when the element's not usefully resizeable. (The spec's "should not apply the resize property" text suggests that we probably shouldn't even show the resizer-thumb.)

So - probably better to actually fix this bug *earlier* in the pipeline, and prevent ourselves from creating the resizer thumb for generated ::before/::after content in the first place.  It seems like we make that decision in nsGfxScrollFrame.cpp, in ScrollFrameHelper::CreateAnonymousContent. In that function, we should ignore the "resize" property (and skip <resizer> creation) if mOuter->GetContent() happens to be a ::before or ::after element.  (probably using the same strategy to test as was described in comment 7, but using mOuter->GetContent()'s NodeInfo.)
Attachment #8760514 - Attachment is obsolete: true
Attachment #8760515 - Attachment is obsolete: true
Comment on attachment 8760514 [details]
Bug 1043537 - Don't show resize handle for generated content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58090/diff/1-2/
Attachment #8760514 - Attachment description: imported patch 1043537-no-resize.patch → Bug 1043537 - Don't show resize handle for pseudo-elements.
Attachment #8760514 - Attachment is obsolete: false
Comment on attachment 8760514 [details]
Bug 1043537 - Don't show resize handle for generated content.

Hi Neil! Would you be able to review this patch? Thanks!
Attachment #8760514 - Flags: review?(enndeakin)
(Neil: ignore the mozreview "interdiff" link in comment 12, when reviewing -- that's comparing an early version of the patch to the final verison.  You can just go directly to https://reviewboard.mozilla.org/r/58090/diff to see the final patch, when reviewing.)
Attachment #8760514 - Flags: review?(enndeakin)
Comment on attachment 8760514 [details]
Bug 1043537 - Don't show resize handle for generated content.

https://reviewboard.mozilla.org/r/58090/#review55460

::: layout/generic/nsGfxScrollFrame.cpp:4251
(Diff revision 2)
>    // Check if the frame is resizable.
>    int8_t resizeStyle = mOuter->StyleDisplay()->mResize;
> -  bool isResizable = resizeStyle != NS_STYLE_RESIZE_NONE;
> +  bool isResizable = resizeStyle != NS_STYLE_RESIZE_NONE &&
> +                     !mOuter->HasAnyStateBits(NS_FRAME_GENERATED_CONTENT);
> +
> +  // "The effect of the resize property on generated content is undefined.

I think this comment should go before the isResizable check

::: layout/reftests/bugs/1043537-2.html:1
(Diff revision 2)
> +<html>

There's already some resizer reftests in http://mxr.mozilla.org/mozilla-central/source/layout/reftests/forms/textarea/ some of which are disabled in some contexts. The try run noted in bug comment 14 only applied to some platforms so make sure we have a proper try test for these new tests.
https://reviewboard.mozilla.org/r/58090/#review55460

> I think this comment should go before the isResizable check

Sounds good, thanks!

> There's already some resizer reftests in http://mxr.mozilla.org/mozilla-central/source/layout/reftests/forms/textarea/ some of which are disabled in some contexts. The try run noted in bug comment 14 only applied to some platforms so make sure we have a proper try test for these new tests.

I see it now! OK, will remove this reftest then.
Comment on attachment 8760514 [details]
Bug 1043537 - Don't show resize handle for generated content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58090/diff/2-3/
Attachment #8760514 - Attachment description: Bug 1043537 - Don't show resize handle for pseudo-elements. → Bug 1043537 - Don't show resize handle for generated content.
Attachment #8760514 - Flags: review?(enndeakin)
Comment on attachment 8760514 [details]
Bug 1043537 - Don't show resize handle for generated content.

https://reviewboard.mozilla.org/r/58090/#review55472
Attachment #8760514 - Flags: review?(enndeakin) → review+
Attachment #8760514 - Attachment is obsolete: true
Attachment #8760514 - Attachment is obsolete: false
(BTW, the Mochitest bc2,bc3,bc4 failures in Comment 19's try run are all known perma-failures from the version-number change. Those tests [which are completely unrelated to this bug] have been disabled or fixed on Trunk, I think; Jonathan was unlucky enough to base his Try run off of a commit that was from before those perma-failures had been addressed.)
Also, historical note: Jonathan accidentally tripped a MozReview foot-gun -- he used a "Discard" option in MozReview to clean up some older review requests, and ended up marking the main review request here as "discarded" (in MozReview).  This "discard" feature is mostly (but not entirely) disabled in MozReview.  Unfortunately, the "un-discard" feature *is* entirely disabled, so there's no going back.

mcote tells me that he'll disable the "discard" avenue that Jonathan ran into, so that others don't run into the same problem.

In the meantime, this bug is in a slightly-awkward state of having a discarded r+'d mozreview request (which e.g. can't be autolanded) -- not a huge deal though, and I'll just directly land the patch on Jonathan's behalf, to avoid confusing sheriffs with the weird mozreview state.
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b412fb33fec
Don't show resize handle for generated content. r=Enn
Try run is pretty much finished, and the oranges appear to all be known-intermittent, infrastructure issues, or the perma-failures which are already addressed on trunk (comment 21).

So, I went ahead and landed (comment 23). Jonathan, congrats on your first patch landing! :)
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/6b412fb33fec
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
"Applies to: elements with overflow other than visible, and optionally replaced elements such as images, videos, and iframes"

https://drafts.csswg.org/css-ui/#resize
percyley, I'm not sure I understand what you're getting at with comment 26.  But, if you read the spec further beyond your quote, you'll find this: "Implementations should not apply the resize property to generated content."  That's precisely what we changed to align with here.
@ Daniel Holbert
I understand that: <img> and other replaced elements should support the resize property.
OK. That is not really part of this bug here. If that doesn't work properly, could you file a new bug with a testcase?
@Daniel Holbert Okay, no problem!
You need to log in before you can comment on or make changes to this bug.