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

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[alp7362]

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

[j.j.]

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

Comment 2

[alp7362]

Attachment: HTML test file

Comment 3

[j.j.]

Attachment: testcase

Comment 4

[alp7362]

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.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 8

[(no longer active)]

Bug 606087 made overflow style changes not apply to inline elements.  This is definitely wrong for text control frames...

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Does backing it out fix things?

Comment 10

[(no longer active)]

Alexander: please see comment 8.

Comment 11

[(no longer active)]

(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.

Comment 12

[(no longer active)]

Attachment: Patch (v1)

Comment 13

[alexander :surkov (:asurkov)]

(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

[alexander :surkov (:asurkov)]

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

Comment 15

[(no longer active)]

(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).

Comment 16

[(no longer active)]

Attachment: Patch (v2)

Accessed the comments on the accessibility test.

Comment 17

[alexander :surkov (:asurkov)]

(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

[alexander :surkov (:asurkov)]

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?

Comment 19

[(no longer active)]

(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.

Comment 20

[(no longer active)]

Attachment: Patch (v3)

Addressed the rest of Alexander's comments (sorry for missing them in the previous round!)

Comment 21

[alexander :surkov (:asurkov)]

(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

[Boris Zbarsky [:bzbarsky]]

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

r=me.  Thank you for grabbing this!

Comment 23

[Mozilla RelEng Bot]

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/[e-mail]-6807bc0d787b

Comment 24

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9013399fa39

Comment 25

[(no longer active)]

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

[alexander :surkov (:asurkov)]

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.

Comment 27

[(no longer active)]

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

[alexander :surkov (:asurkov)]

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.

Comment 29

[(no longer active)]

Relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/2705abe8d3f2

Comment 30

[(no longer active)]

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

[alexander :surkov (:asurkov)]

(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

[alexander :surkov (:asurkov)]

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.

Comment 33

[(no longer active)]

(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

Comment 34

[(no longer active)]

Third time's a charm.  This stuck this time:

https://hg.mozilla.org/mozilla-central/rev/c2587d34df08