Last Comment Bug 686247 - <textarea>s no longer honor some combinations of dynamic changes to 'overflow' (between visible/clip and scroll/auto/hidden)
: <textarea>s no longer honor some combinations of dynamic changes to 'overflow...
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: :Ehsan Akhgari (busy, don't ask for review please)
:
Mentors:
Depends on:
Blocks: 534785 606087
  Show dependency treegraph
 
Reported: 2011-09-11 17:42 PDT by alp7362
Modified: 2011-09-15 07:40 PDT (History)
7 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
HTML test file (387 bytes, text/html)
2011-09-11 18:08 PDT, alp7362
no flags Details
testcase (485 bytes, text/html)
2011-09-11 19:23 PDT, j.j.
no flags Details
Patch (v1) (9.29 KB, patch)
2011-09-12 08:59 PDT, :Ehsan Akhgari (busy, don't ask for review please)
no flags Details | Diff | Review
Patch (v2) (9.54 KB, patch)
2011-09-12 11:25 PDT, :Ehsan Akhgari (busy, don't ask for review please)
surkov.alexander: review+
Details | Diff | Review
Patch (v3) (9.70 KB, patch)
2011-09-12 18:41 PDT, :Ehsan Akhgari (busy, don't ask for review please)
bzbarsky: review+
Details | Diff | Review

Description alp7362 2011-09-11 17:42:30 PDT
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. 2011-09-11 17:53:28 PDT
Please attach a simple HTML test file (use the "Add an attachment" link above)
Comment 2 alp7362 2011-09-11 18:08:36 PDT
Created attachment 559807 [details]
HTML test file
Comment 3 j.j. 2011-09-11 19:23:18 PDT
Created attachment 559811 [details]
testcase
Comment 4 alp7362 2011-09-11 19:33:58 PDT
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-7 (review requests must explain patch) 2011-09-11 19:40:47 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-12 08:07:07 PDT
Bug 606087 made overflow style changes not apply to inline elements.  This is definitely wrong for text control frames...
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-09-12 08:29:02 PDT
Er, yes.  Replaced elements mean that patch is probably just wrong.  :(

Does backing it out fix things?
Comment 10 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-12 08:40:56 PDT
Alexander: please see comment 8.
Comment 11 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-12 08:41:55 PDT
(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 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-12 08:59:21 PDT
Created attachment 559866 [details] [diff] [review]
Patch (v1)
Comment 13 alexander :surkov 2011-09-12 08:59:56 PDT
(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 2011-09-12 09:05:40 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-12 10:52:00 PDT
(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 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-12 11:25:44 PDT
Created attachment 559881 [details] [diff] [review]
Patch (v2)

Accessed the comments on the accessibility test.
Comment 17 alexander :surkov 2011-09-12 16:04:54 PDT
(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 2011-09-12 16:08:23 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-12 18:38:10 PDT
(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 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-12 18:41:08 PDT
Created attachment 559919 [details] [diff] [review]
Patch (v3)

Addressed the rest of Alexander's comments (sorry for missing them in the previous round!)
Comment 21 alexander :surkov 2011-09-12 18:59:24 PDT
(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 [:bz] (Out June 25-July 6) 2011-09-12 21:39:49 PDT
Comment on attachment 559919 [details] [diff] [review]
Patch (v3)

r=me.  Thank you for grabbing this!
Comment 23 Mozilla RelEng Bot 2011-09-13 03:10:58 PDT
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
Comment 24 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-13 08:46:55 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9013399fa39
Comment 25 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-13 10:32:11 PDT
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 2011-09-13 11:27:26 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-13 11:49:37 PDT
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 2011-09-13 11:55:40 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-13 12:12:01 PDT
Relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/2705abe8d3f2
Comment 30 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-13 14:45:27 PDT
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 2011-09-13 19:43:23 PDT
(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 2011-09-14 19:57:37 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-14 21:08:28 PDT
(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 :Ehsan Akhgari (busy, don't ask for review please) 2011-09-15 07:40:16 PDT
Third time's a charm.  This stuck this time:

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

Note You need to log in before you can comment on or make changes to this bug.