Summary: | Non-resizable text field looks resizable | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Cosimo Cecchi <cosimoc> | ||||||||||||
Component: | Forms | Assignee: | 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
Cosimo Cecchi
2015-12-14 14:20:26 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. FWIW this seems to work fine in Chrome. I think it’s a bug. Minimized test case: <input type="text" style="resize:vertical"> (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.. Created attachment 276320 [details]
Patch v1
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.
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. Created attachment 276323 [details]
Patch v1.1 - improved changelog.
(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. (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. (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. 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 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). (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. (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. Created attachment 276344 [details]
Patch v1.3 - for landing.
Comment on attachment 276344 [details] Patch v1.3 - for landing. Clearing flags on attachment: 276344 Committed r199512: <http://trac.webkit.org/changeset/199512> All reviewed patches have been landed. Closing bug. |