Note: There are a few cases of duplicates in user autocompletion which are being worked on.

<textarea>s no longer honor some combinations of dynamic changes to 'overflow' (between visible/clip and scroll/auto/hidden)

RESOLVED FIXED in mozilla9

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: alp7362, Assigned: Ehsan)

Tracking

({regression})

Trunk
mozilla9
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
User Agent: Opera/9.80 (Windows NT 5.1; U; en) Presto/2.9.168 Version/11.51

Steps to reproduce:

I set the overflow property to hidden for a textarea element via JavaScript. (I wrote about it in details at http://stackoverflow.com/q/7371863)


Actual results:

But it does not get applied. The overflowing content of the textarea element is not clipped so the srollbars appear. 


Expected results:

If the content overflows a textarea element and the overflow property is set to hidden, the content should be clipped and no scrollbars appear in the textarea element.

Comment 1

6 years ago
Please attach a simple HTML test file (use the "Add an attachment" link above)
(Reporter)

Comment 2

6 years ago
Created attachment 559807 [details]
HTML test file

Updated

6 years ago
Attachment #559807 - Attachment mime type: text/plain → text/html

Comment 3

6 years ago
Created attachment 559811 [details]
testcase

Updated

6 years ago
Component: General → Layout
OS: Windows XP → All
Product: Firefox → Core
QA Contact: general → layout
Hardware: x86 → All
Version: 5 Branch → Trunk

Updated

6 years ago
Summary: Overflow: hidden does not apply when set via JavaScript → Overflow: hidden does not apply to <textarea> when set via JavaScript onload
(Reporter)

Comment 4

6 years ago
Comment on attachment 559811 [details]
testcase

I don't know how it appears to be in the other versions but in Firefox 5 the first box does appear with the vertical scrollbar.
Component: Layout → Layout: Form Controls
QA Contact: layout → layout.form-controls
The underlying problem is that nsTextEditorState::CreateRootNode (in content/html/content/src/nsTextEditorState.cpp) sets the 'inherit-overflow' class depending on the static value of overflow, and that class is then unchanged if the value of overflow changes.

I presume this is a regression from bug 534785.  (When the anonymous content was attached to the frame this wasn't a problem since 'overflow' changes caused frame reconstruction.)
Blocks: 534785
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Summary: Overflow: hidden does not apply to <textarea> when set via JavaScript onload → <textarea>s no longer honor some combinations of dynamic changes to 'overflow' (between visible/clip and scroll/auto/hidden)
Bug 606087 made overflow style changes not apply to inline elements.  This is definitely wrong for text control frames...
Assignee: nobody → ehsan
Blocks: 606087

Comment 9

6 years ago
Er, yes.  Replaced elements mean that patch is probably just wrong.  :(

Does backing it out fix things?
Alexander: please see comment 8.
(In reply to Boris Zbarsky (:bz) from comment #9)
> Er, yes.  Replaced elements mean that patch is probably just wrong.  :(
> 
> Does backing it out fix things?

Backing out that patch is part of what needs to be done here, yes.
Created attachment 559866 [details] [diff] [review]
Patch (v1)
Attachment #559866 - Flags: review?(bzbarsky)

Comment 13

6 years ago
(In reply to Ehsan Akhgari [:ehsan] from comment #10)
> Alexander: please see comment 8.

A11y concern was to not recreate frames on focus when nothing visually is changed, it sounds like it makes sense for frame tree as well. This problem isn't quite bad for a11y but leads to unexpected behavior for AT developers (I think it still should be ok on user side). 

If you decide to back out this patch completely then I think a11y could live with that but eventually we should end up with code logic movement of this sort into a11y part since we shouldn't recreate accessible for every frame tree change.

Comment 14

6 years ago
Comment on attachment 559866 [details] [diff] [review]
Patch (v1)

Review of attachment 559866 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/tests/mochitest/tree/test_cssoverflow.html
@@ -37,5 @@
>  
> -      this.unexpectedEventSeq = [
> -        new invokerChecker(EVENT_REORDER, this.linkNode.parentNode)
> -      ];
> -

rather than remove it, it's worth to add asyncInvokerChecker for reorder event into eventSeq

@@ -46,5 @@
>  
>        this.check = function focusAnchor_check(aEvent)
>        {
> -        is(this.link, aEvent.accessible,
> -           "The link accessible shouldn't be recreated!");

I'd like to see todo here as well, since this is not desired behavior

@@ -76,5 @@
>  
>        this.check = function tabAnchor_check(aEvent)
>        {
> -        is(this.link, aEvent.accessible,
> -           "The link accessible shouldn't be recreated!");

same for this case

@@ -128,5 @@
> -  <a target="_blank"
> -     title="Don't recreate frames for inlines with overflow style applied"
> -     href="https://bugzilla.mozilla.org/show_bug.cgi?id=606087">
> -    Mozilla Bug 606087
> -  </a><br>

please add the bug reference for this bug
(In reply to alexander surkov from comment #13)
> (In reply to Ehsan Akhgari [:ehsan] from comment #10)
> > Alexander: please see comment 8.
> 
> A11y concern was to not recreate frames on focus when nothing visually is
> changed, it sounds like it makes sense for frame tree as well. This problem
> isn't quite bad for a11y but leads to unexpected behavior for AT developers
> (I think it still should be ok on user side). 
> 
> If you decide to back out this patch completely then I think a11y could live
> with that but eventually we should end up with code logic movement of this
> sort into a11y part since we shouldn't recreate accessible for every frame
> tree change.

In the general case, it is definitely possible for a reframe to not cause any visual changes on the screen, so if the accessibility module does not want to recreate accessible objects for that case, this needs to be handled at another level.  For this specific change, changes in the overflow style actually can change what appears on the screen for inline elements (for example, textareas).
Created attachment 559881 [details] [diff] [review]
Patch (v2)

Accessed the comments on the accessibility test.
Attachment #559866 - Attachment is obsolete: true
Attachment #559866 - Flags: review?(bzbarsky)
Attachment #559881 - Flags: review?(surkov.alexander)
Attachment #559881 - Flags: review?(bzbarsky)

Comment 17

6 years ago
(In reply to Ehsan Akhgari [:ehsan] from comment #15)

> In the general case, it is definitely possible for a reframe to not cause
> any visual changes on the screen, so if the accessibility module does not
> want to recreate accessible objects for that case, this needs to be handled
> at another level.  For this specific change, changes in the overflow style
> actually can change what appears on the screen for inline elements (for
> example, textareas).

Still miss why would you need to reframe in general case, is it because that reframing is cheaper than checks to prevent it?

I would appreciate if you point how can detect these cases in a11y. Layout notifies us about frame creation/desctruction via nsAccessibility::ContentRangeInserted/ContentRemoved. Any ideas how can I prevent this pair to trigger or how can I filter cases inside this pair?

Anyway, could you please file a bug for that?

Comment 18

6 years ago
Comment on attachment 559881 [details] [diff] [review]
Patch (v2)

Review of attachment 559881 [details] [diff] [review]:
-----------------------------------------------------------------

for a11y part

::: accessible/tests/mochitest/tree/test_cssoverflow.html
@@ -66,5 @@
>        ];
>  
> -      this.unexpectedEventSeq = [
> -        new invokerChecker(EVENT_REORDER, this.linkNode.parentNode)
> -      ];

put this reorder as asyncInvokerChecker too please

@@ +69,5 @@
>  
>        this.check = function tabAnchor_check(aEvent)
>        {
> +        isnot(this.link, aEvent.accessible,
> +              "Focus should be fired against new link accessible!");

todo_is?
Attachment #559881 - Flags: review?(surkov.alexander) → review+
(In reply to alexander surkov from comment #17)
> (In reply to Ehsan Akhgari [:ehsan] from comment #15)
> 
> > In the general case, it is definitely possible for a reframe to not cause
> > any visual changes on the screen, so if the accessibility module does not
> > want to recreate accessible objects for that case, this needs to be handled
> > at another level.  For this specific change, changes in the overflow style
> > actually can change what appears on the screen for inline elements (for
> > example, textareas).
> 
> Still miss why would you need to reframe in general case, is it because that
> reframing is cheaper than checks to prevent it?
> 
> I would appreciate if you point how can detect these cases in a11y. Layout
> notifies us about frame creation/desctruction via
> nsAccessibility::ContentRangeInserted/ContentRemoved. Any ideas how can I
> prevent this pair to trigger or how can I filter cases inside this pair?
> 
> Anyway, could you please file a bug for that?

Sure, I filed bug 686400.

The reason that we need to reframe in more cases than what is absolutely necessary is that when we are computing what needs to happen because of a style change, we do not have enough information to say for sure whether something needs to change on the screen or not.  Moreover, even if we could, the element might be reframed because of a style change in one of its ancestors.

By the time you get the ContentRangeInserted/Removed notifications, it's too late to figure out why this is happening (I think).  I really don't know a solution right now, but let's try to figure out a general solution in bug 686400.

Also, note that reframes are (mostly) cheap, so we reframe in lots of situations where technically the layout will not change, so a reframe might not be quite necessary, I believe.
Created attachment 559919 [details] [diff] [review]
Patch (v3)

Addressed the rest of Alexander's comments (sorry for missing them in the previous round!)
Attachment #559881 - Attachment is obsolete: true
Attachment #559881 - Flags: review?(bzbarsky)
Attachment #559919 - Flags: review?(bzbarsky)

Comment 21

6 years ago
(In reply to Ehsan Akhgari [:ehsan] from comment #19)

> Sure, I filed bug 686400.

Thank you.

> By the time you get the ContentRangeInserted/Removed notifications, it's too
> late to figure out why this is happening (I think).  I really don't know a
> solution right now, but let's try to figure out a general solution in bug
> 686400.

Ok, if you run into any ideas please share it.

Comment 22

6 years ago
Comment on attachment 559919 [details] [diff] [review]
Patch (v3)

r=me.  Thank you for grabbing this!
Attachment #559919 - Flags: review?(bzbarsky) → review+

Comment 23

6 years ago
Try run for 6807bc0d787b is complete.
Detailed breakdown of the results available here:
    http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=6807bc0d787b
Results (out of 171 total builds):
    exception: 3
    success: 148
    warnings: 7
    failure: 13
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-6807bc0d787b
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9013399fa39
Flags: in-testsuite+
Target Milestone: --- → mozilla9
I backed this out https://hg.mozilla.org/integration/mozilla-inbound/rev/c9013399fa39 because of Linux accessibility test failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=6387284&tree=Mozilla-Inbound&full=1

It seems to me that the failures were caused by the changes that I made in response to comment 18, because the try job that I pushed yesterday without these changes did not have these failures (see comment 23).

Alexander, can you please take a look at these failures, and let me know what I need to do?  Thanks!

Comment 26

6 years ago
Two reorder events for link parent. Looks like intermittent failure (no failures on try, linux only, the second test only). Could it be because ContentRemoved and ContentRangeInserted for that link frame triggers not in sync so that nsARefreshObserver::WillRefresh for Flush_Display has a chance to be called between them? If so then it makes sense to remove reorder event from expected events.
This wasn't Linux only, it turns out that it also fails on Windows <https://tbpl.mozilla.org/?tree=Mozilla-Inbound&usebuildbot=1&rev=c9013399fa39>.  And it doesn't look intermittent...

Can I reland the patch without the reorder event?

Comment 28

6 years ago
They are rather nice to have than principal thing, especially in the light of bug 686400. But I'm curious is that something we should investigate further and put one more todo or my guess can be true and then no todo is needed.
Relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/2705abe8d3f2
I had to back this out again because of a new failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=6389721&tree=Mozilla-Inbound&full=1

Alexander, would you mind applying the patch in your tree and investigating the failure?  I really don't know what the expected behavior here is, and how to address this test failure.  Thanks!

Comment 31

6 years ago
(In reply to Ehsan Akhgari [:ehsan] from comment #30)
> I had to back this out again because of a new failure:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=6389721&tree=Mozilla-
> Inbound&full=1
> 
> Alexander, would you mind applying the patch in your tree and investigating
> the failure?  I really don't know what the expected behavior here is, and
> how to address this test failure.  Thanks!

theoretically two focus events may be fired, one is for old accessible, one is for new one, that depends on timing when we notified about DOM event, ContentRemoved and ContentRangeInserted. If we have two focus then the "check" function triggers on the first one and reports that accessible wasn't recreated. That is something bogus and regression. If this is valid scenario then I would try to find a fix for bug 686400 rather than spend a time to find a workaround of this problem. I'll check if my guess is correct.

Comment 32

6 years ago
Ehsan, nothing wrong with logic, you have type-o in mochitest, todo_isnot instead of todo_is.

Also, please remove those two todo(false, "Doubled reorder events"). I checked and don't see anything wrong with events. It appears my guess from comment #26 is valid.
(In reply to alexander surkov from comment #32)
> Ehsan, nothing wrong with logic, you have type-o in mochitest, todo_isnot
> instead of todo_is.

Ah, right!

> Also, please remove those two todo(false, "Doubled reorder events"). I
> checked and don't see anything wrong with events. It appears my guess from
> comment #26 is valid.

Sure, will do.  I've landed the patch again with these changes:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2587d34df08
Third time's a charm.  This stuck this time:

https://hg.mozilla.org/mozilla-central/rev/c2587d34df08
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.