Bug 152271

Summary: Non-resizable text field looks resizable
Product: WebKit Reporter: Cosimo Cecchi <cosimoc>
Component: FormsAssignee: Antonio Gomes <tonikitoo>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, cgarcia, commit-queue, darin, esprehn+autocc, glenn, kling, koivisto, kondapallykalyan, mcatanzaro, simon.fraser
Priority: P2 Keywords: WebExposed
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Linux   
URL: data:text/html,<input type="text" style="resize:vertical">
See Also: https://bugzilla.gnome.org/show_bug.cgi?id=759370
Bug Depends on: 150550    
Bug Blocks:    
Attachments:
Description Flags
screenshot
none
Patch v1
none
Patch v1.1 - improved changelog.
none
Patch v1.2 - removed the git-generated text on the patch.
darin: review+
Patch v1.3 - for landing. none

Description Cosimo Cecchi 2015-12-14 14:20:26 PST
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.
Comment 1 Carlos Garcia Campos 2016-02-18 06:29:17 PST
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.
Comment 2 Cosimo Cecchi 2016-02-18 08:36:07 PST
FWIW this seems to work fine in Chrome.
Comment 3 Darin Adler 2016-02-25 09:42:08 PST
I think it’s a bug. Minimized test case:

<input type="text" style="resize:vertical">
Comment 4 Antonio Gomes 2016-04-05 13:51:46 PDT
(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..
Comment 5 Antonio Gomes 2016-04-13 07:35:32 PDT
Created attachment 276320 [details]
Patch v1
Comment 6 Darin Adler 2016-04-13 08:30:04 PDT
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.
Comment 7 Darin Adler 2016-04-13 08:31:43 PDT
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.
Comment 8 Antonio Gomes 2016-04-13 08:37:16 PDT
Created attachment 276323 [details]
Patch v1.1 - improved changelog.
Comment 9 Antonio Gomes 2016-04-13 08:39:52 PDT
(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.
Comment 10 Darin Adler 2016-04-13 08:46:00 PDT
(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.
Comment 11 Carlos Garcia Campos 2016-04-13 08:48:36 PDT
(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.
Comment 12 Antonio Gomes 2016-04-13 08:51:27 PDT
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.
Comment 13 Darin Adler 2016-04-13 09:21:19 PDT
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).
Comment 14 Darin Adler 2016-04-13 09:40:10 PDT
(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.
Comment 15 Antonio Gomes 2016-04-13 12:40:13 PDT
(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.
Comment 16 Antonio Gomes 2016-04-13 12:41:08 PDT
Created attachment 276344 [details]
Patch v1.3 - for landing.
Comment 17 WebKit Commit Bot 2016-04-13 13:40:00 PDT
Comment on attachment 276344 [details]
Patch v1.3 - for landing.

Clearing flags on attachment: 276344

Committed r199512: <http://trac.webkit.org/changeset/199512>
Comment 18 WebKit Commit Bot 2016-04-13 13:40:04 PDT
All reviewed patches have been landed.  Closing bug.