Bug 152271 – Non-resizable text field looks resizable

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Cosimo Cecchi]

Created attachment 267321 [details]
screenshot

Originally reported at https://bugzilla.gnome.org/show_bug.cgi?id=759370


In the attached screenshot, the "Description:" text field looks resizable (has a resize grip), but it's not, and it doesn't look like it should be.

[Carlos Garcia Campos]

I'm not sure there's any bug here. That field has the following code:

<input type="text" id="description" name="description" class="required" size="60" maxlength="200">

And in the attachment.css there's:

#description {
    resize: vertical;
}

So, this a bugzilla issue, I would say. It's true that we could probably check that the input is actually resizable and ignore the resize style otherwise. Also removing the GTK prefix because this is not specific to GTK port.

[Cosimo Cecchi]

FWIW this seems to work fine in Chrome.

[Darin Adler]

I think it’s a bug. Minimized test case:

<input type="text" style="resize:vertical">

[Antonio Gomes]

(In reply to comment #3)
> I think it’s a bug. Minimized test case:
> 
> <input type="text" style="resize:vertical">

Reproducible on Safari nightly, as of 2016/April/5. Taking..

[Antonio Gomes]

Created attachment 276320 [details]
Patch v1

[Darin Adler]

Comment on attachment 276320 [details]
Patch v1

Can’t use bugs.webkit.org’s Review Patch on this because it’s email-formatted, not just a patch. Looks like it will likely be a good fix, though.

[Darin Adler]

I’m surprised that the inheritance is the complete cause of this issue. I somehow thought there was also a separate “style asks for it to be resizable but isn’t really in practice” issue, but maybe not.

[Antonio Gomes]

Created attachment 276323 [details]
Patch v1.1 - improved changelog.

[Antonio Gomes]

(In reply to comment #7)
> I’m surprised that the inheritance is the complete cause of this issue. I
> somehow thought there was also a separate “style asks for it to be resizable
> but isn’t really in practice” issue, but maybe not.

Thanks for looking, Darin.

Right, my understanding is that the current inheritance nature of the property caused the issue.

And yes, for some reason the "review patch" link is also broken to me too.

[Darin Adler]

(In reply to comment #9)
> And yes, for some reason the "review patch" link is also broken to me too.

The reason is that the uploaded patch is not a patch, it’s an email with headers containing a patch. bugs.webkit.org doesn’t support that. Please leave out all the git-generated email stuff and upload just a patch.

[Carlos Garcia Campos]

(In reply to comment #10)
> (In reply to comment #9)
> > And yes, for some reason the "review patch" link is also broken to me too.
> 
> The reason is that the uploaded patch is not a patch, it’s an email with
> headers containing a patch. bugs.webkit.org doesn’t support that. Please
> leave out all the git-generated email stuff and upload just a patch.

That's how git format-patch generates patches and I think it has always worked with WebKit bugzilla.

[Antonio Gomes]

Created attachment 276324 [details]
Patch v1.2 - removed the git-generated text on the patch.

Manually removed the Git header. Lets see if bugzilla receives it better this time.

Btw, I have been uploading patches this way, and it used to work.

[Darin Adler]

Comment on attachment 276324 [details]
Patch v1.2 - removed the git-generated text on the patch.

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

There are effectively two different changes here:

1) Fix shadow implementation of <input> to not inherit the CSS resize property. That’s what the bug was reporting.

2) Change the CSS resize property, a CSS property that was first proposed by and implemented in WebKit, to not be inheritable, even though it was presumably always inheritable in all versions of WebKit. Apparently the specification and all other engines decided that it was better to have it not be inheritable. And this does sound reasonable, but with years of content being built and much only tested with WebKit, there’s a chance that someone relies on the 10-year-old behavior that WebKit has always had.

I think it’s OK to make this change, but there is some risk that it will break some existing content that has only been tested with WebKit.

> Source/WebCore/ChangeLog:23
> +        On WebKit, this happens because the 'resize' property is wrongly implemented as 'inherable',

Typo here: "inheritable".

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:228
> +    unsigned m_resize : 2; // EResize

Peculiar that all these public data members have the m_ prefix here. Public data members should almost never have the prefix, and note that many above do not have it (marginAfterCollapse, for example).

[Darin Adler]

(In reply to comment #12)
> Btw, I have been uploading patches this way, and it used to work.

Should ask on webkit-dev. Maybe someone knows why it stopped working.

[Antonio Gomes]

(In reply to comment #13)
> Comment on attachment 276324 [details]
> Patch v1.2 - removed the git-generated text on the patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276324&action=review
> 
> There are effectively two different changes here:
> 
> 1) Fix shadow implementation of <input> to not inherit the CSS resize
> property. That’s what the bug was reporting.
> 
> 2) Change the CSS resize property, a CSS property that was first proposed by
> and implemented in WebKit, to not be inheritable, even though it was
> presumably always inheritable in all versions of WebKit. Apparently the
> specification and all other engines decided that it was better to have it
> not be inheritable. And this does sound reasonable, but with years of
> content being built and much only tested with WebKit, there’s a chance that
> someone relies on the 10-year-old behavior that WebKit has always had.
> 

This is certainly the case, Darin.

If it turns out the there is lots of WebKit-only content that rely on this WebKit quirk I would happily provide a more restrictive solution (see below).

> I think it’s OK to make this change, but there is some risk that it will
> break some existing content that has only been tested with WebKit.

For instance, an alternative way to fix this and only affect <input> HTML elements and alike, could be to patch RenderLayer::canResize(), like below (pseudo-code):

+ static bool forciblyDisallowResize(RenderElement& renderer)
+ {
+    auto* root = getShadowRoot(renderer);
+    return is<RenderTextSingleLine(root));
+ }

 bool RenderLayer::canResize() const
 {
     // We need a special case for <iframe> because they never have
     // hasOverflowClip(). However, they do "implicitly" clip their contents, so
    // we want to allow resizing them also.
-    return (renderer().hasOverflowClip() || renderer().isRenderIFrame()) && renderer().style().resize() != RESIZE_NONE;
+    return (renderer().hasOverflowClip() || renderer().isRenderIFrame()) && renderer().style().resize() != RESIZE_NONE && forciblyDisallowResize(renderer());
 }
 
> > Source/WebCore/ChangeLog:23
> > +        On WebKit, this happens because the 'resize' property is wrongly implemented as 'inherable',
> 
> Typo here: "inheritable".

Fixed.
 
> > Source/WebCore/rendering/style/StyleRareNonInheritedData.h:228
> > +    unsigned m_resize : 2; // EResize
> 
> Peculiar that all these public data members have the m_ prefix here. Public
> data members should almost never have the prefix, and note that many above
> do not have it (marginAfterCollapse, for example).

Noticed this too. This mix of style is a bit annoying.

[Antonio Gomes]

Created attachment 276344 [details]
Patch v1.3 - for landing.

[WebKit Commit Bot]

Comment on attachment 276344 [details]
Patch v1.3 - for landing.

Clearing flags on attachment: 276344

Committed r199512: <http://trac.webkit.org/changeset/199512>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.