Closed Bug 1423331 Opened 7 years ago Closed 7 years ago

telegram text box cursor is confused when focusing both window and textbox in 59.0a1 (2017-12-05) (64-bit)

Categories

(Core :: Layout, defect, P2)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: bkelly, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(4 files)

Screenshot 2017-12-05 15.11.19.png
69.74 KB, image/png
Details
Testcase
252 bytes, text/html
Details
Bug 1423331: Prevent selection from leaking from blocks.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1423331: Tests.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details 
I use the desktop web version of telegram at:

https://web.telegram.org

After updating today I noticed that its extremely difficult to type messages.  When I click in the text box often the cursor ends up on the far right of the box.  If I click around enough eventually I can get it positioned on the left and I can start typing.

Comparing to the site on firefox beta it seems the placeholder text "Write a message..." is not getting removed when I click in the textbox.  On beta the text is removed immediately when clicking in the box.  In nightly it does not.

See the attached screenshot.  Note the cursor placement.

This only seems to effect nightly.  Beta 58 does not seem to have the problem AFAICT.
It seems an effect due to clicking both into the textbox and into the window at the same time.  If I'm already focusing the window when I click the textbox then I don't have any problems and the cursor is placed on the left.
Summary: telegram text box does not clear placeholder text on input in 59.0a1 (2017-12-05) (64-bit) → telegram text box cursor is confused when focusing both window and textbox in 59.0a1 (2017-12-05) (64-bit)
Did it just start happening yesterday? ISTR some recent editor work around a cursor position change regression sites. Makoto or Masayuki, do you recall that bug?
Flags: needinfo?(masayuki)
Flags: needinfo?(m_kato)
Priority: -- → P2
Yes, I believe it just started yesterday.  Maybe the day before.  Note, I often go a week or two between updates of nightly, though.
According to the timing, this might be caused by bug 1421504. But I don't understand how affects this bug since you don't modify editor by yourself.
Flags: needinfo?(masayuki)
According to mozregression, this is a regression of bug 1035091.

Actually, https://web.telegram.org/css/app.css has this line:
> @-moz-document url-prefix(){.composer_rich_textarea:empty:active:before,.composer_rich_textarea:empty:focus:before{display:none}}

So, Tech Evangelism bug?
Blocks: 1035091
Flags: needinfo?(m_kato)
Flags: needinfo?(emilio)
Thanks for tracking that down, Masayuki! I'll wait for Emilio to respond to his own needinfo before moving this to TE.
Attached file Testcase — 
So, yeah, there's a tech evangelism bug to some extent, which is that removing @-moz-document broke their hack.

But I investigated a bit, and the reason they need this hack is a bug of ours I think...

Here's a test-case where we mess up the insertion point pretty randomly, and are no longer able to edit the message. It's reduced from Telegram.

So maybe we should split this bug into an editor bug and an evangelism bug?
Assignee: nobody → emilio
Flags: needinfo?(emilio)
Err, didn't intend to take, at least yet... I'll poke a bit though.
Assignee: emilio → nobody
Hmm, I'm really not familiar with around the patch. Could you find another reviewer of layout?
Huh, I chose you looking at blame, you're the only person actively working on FF that touched that code "recently" (bug 550434).

I guess I can try to find someone during the All Hands...
Comment on attachment 8935836 [details]
Bug 1423331: Prevent selection from leaking from blocks.

Hmm, okay. If you won't find better reviewer, feel free to re-request me to review.
Attachment #8935836 - Flags: review?(masayuki)
mats might have some opinion on this.
Still the same behavior in 59.0a1 (2017-12-12) (64-bit)
(In reply to box4den from comment #16)
> Still the same behavior in 59.0a1 (2017-12-12) (64-bit)

And if that helps in any way https://web.whatsapp.com/ works ok.
Attachment #8935810 - Attachment mime type: text/plain → text/html
Here Be Dragons...
FYI, this part of the code is possibly the most regression-prone
part of Gecko.  Any changes to it, no matter how trivial, pretty
much always breaks something.

So, for that reason I need to understand in detail what goes
wrong here before reviewing anything.  I'd like a detailed
explanation of the series of events that leads to the error
situation (including frame dumps / stacks), please.
Flags: needinfo?(emilio)
So we're getting the offsets from:

#0  0x00007fd25a63a608 in GetSelectionClosestFrameForBlock(nsIFrame*, nsPoint const&, unsigned int) (aFrame=0x7fd21f19bac0, aPoint=..., aFlags=2)
    at /home/emilio/projects/moz/stylo/layout/generic/nsFrame.cpp:4974
#1  0x00007fd25a622d53 in GetSelectionClosestFrame(nsIFrame*, nsPoint const&, unsigned int) (aFrame=0x7fd21f19bac0, aPoint=..., aFlags=2)
    at /home/emilio/projects/moz/stylo/layout/generic/nsFrame.cpp:4992
#2  0x00007fd25a621050 in nsIFrame::GetContentOffsetsFromPoint(nsPoint const&, unsigned int) (this=0x7fd21f19bac0, aPoint=..., aFlags=2)
    at /home/emilio/projects/moz/stylo/layout/generic/nsFrame.cpp:5110
#3  0x00007fd25a622105 in nsFrame::HandleRelease(nsPresContext*, mozilla::WidgetGUIEvent*, nsEventStatus*) (this=0x7fd21f19bac0, aPresContext=0x7fd23228f000, aEvent=0x7fff5d4c3e00, aEventStatus=0x7fff5d4c2ac8) at /home/emilio/projects/moz/stylo/layout/generic/nsFrame.cpp:4637
#4  0x00007fd25a620066 in nsFrame::HandleEvent(nsPresContext*, mozilla::WidgetGUIEvent*, nsEventStatus*) (this=0x7fd21f19bac0, aPresContext=0x7fd23228f000, aEvent=0x7fff5d4c3e00, aEventStatus=0x7fff5d4c2ac8) at /home/emilio/projects/moz/stylo/layout/generic/nsFrame.cpp:3876
#5  0x00007fd25a4ddc78 in nsPresShellEventCB::HandleEvent(mozilla::EventChainPostVisitor&) (this=0x7fff5d4c2f10, aVisitor=...)
    at /home/emilio/projects/moz/stylo/layout/base/PresShell.cpp:512
#6  0x00007fd258e69c56 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) (aChain=nsTArray<mozilla::EventTargetChainItem> & = {...}, aVisitor=..., aCallback=0x7fff5d4c2f10, aCd=...)
    at /home/emilio/projects/moz/stylo/dom/events/EventDispatcher.cpp:508
#7  0x00007fd258e6b01d in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) (aTarget=0x7fd21ca340d0, aPresContext=0x7fd23228f000, aEvent=0x7fff5d4c3e00, aDOMEvent=0x0, aEventStatus=0x7fff5d4c3c98, aCallback=0x7fff5d4c2f10, aTargets=0x0)
    at /home/emilio/projects/moz/stylo/dom/events/EventDispatcher.cpp:826

At that point adjustedFrame is the <div> primary frame. The relevant frame tree looks like:

          Block(body)(1)@7fd21f19b9c0 parent=7fd21f19b910 {480,480,75840,6120} vis-overflow=-60,-60,75900,6240 scr-overflow=0,0,75840,6120 [state=0000120000100200] [content=7fd21ecee310] [sc=7fd21c8c4cd8]<
            line 7fd21f19bd48: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x8] {0,0,6120,6120} vis-overflow=-60,-60,6240,6240 scr-overflow=0,0,6120,6120 <
              Block(div)(0)@7fd21f19bac0 parent=7fd21f19b9c0 {0,0,6120,6120} vis-overflow=-60,-60,6240,6240 [state=000012c000100210] [content=7fd21ca340d0] [sc=7fd21ca65bd8]<
                line 7fd21f19bcf8: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x8] {60,60,6000,1140} <
                  Block(_moz_generated_content_before)(-1)@7fd21f19bb70 parent=7fd21f19bac0 {60,60,6000,1140} [state=0000108000100240] [content=7fd21ca341f0] [sc=7fd21c8c4ef8:before]<
                    line 7fd21f19bca8: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x100] {0,0,5190,1140} <
                      Text(0)"Write here"@7fd21f19bc20 parent=7fd21f19bb70 {0,0,5190,1140} [state=00000040b0600040] [content=7fd21ecf5880] [sc=7fd21c8c4568:-moz-text] [run=7fd21ca34310][0,10,T] 
                    >
                  >
                >
              >
            >
          >

In the "success" case where the hack they have applies, hiding the ::before pseudo-element, there's no ::before frame, and thus we go through:

  https://searchfox.org/mozilla-central/rev/9d920555ec81f1c9d4b7fa3b08e23eb88efb60e1/layout/generic/nsFrame.cpp#4914

But in this case there's a ::before pseudo frame, which isn't selectable, and thus we land to:

  https://searchfox.org/mozilla-central/rev/9d920555ec81f1c9d4b7fa3b08e23eb88efb60e1/layout/generic/nsFrame.cpp#4976

That ends up in:

  https://searchfox.org/mozilla-central/rev/9d920555ec81f1c9d4b7fa3b08e23eb88efb60e1/layout/generic/nsFrame.cpp#4835

So the returned frame target does have the frameEdge flag, and the afterFrame flag, but not the emptyBlock flag, and the target is `this`:

(rr) p closest
$5 = {frame = 0x7fd21f19bac0, frameEdge = true,	afterFrame = true, emptyBlock = false}
(rr) p closest.frame
$6 = (nsBlockFrame *) 0x7fd21f19bac0
(rr) p closest.frame->GetContent()
$7 = (mozilla::dom::HTMLDivElement *) 0x7fd21ca340d0
(rr) p this
$8 = (nsBlockFrame *) 0x7fd21f19bac0

Which means that we end up in:

  https://searchfox.org/mozilla-central/rev/9d920555ec81f1c9d4b7fa3b08e23eb88efb60e1/layout/generic/nsFrame.cpp#5127

However, GetRangeForFrame returns actually the _parent_ of the block, the <body> frame, which is what causes the selection to get lost and what not, and then causes us to ultimately hit:

  https://searchfox.org/mozilla-central/rev/9d920555ec81f1c9d4b7fa3b08e23eb88efb60e1/dom/events/IMEContentObserver.cpp#1604

As I said, I'm not convinced this patch is the totally real fix (maybe GetRangeForParent should return the content itself in this case? That seemed like a more risky change). It does make behavior consistent when all the selectable kids of a block are anonymous content / non-selectable, and when it's empty (which was the fix of bug 550434).
Flags: needinfo?(emilio)
Thanks for documenting the details.  Your reasoning makes sense to me, so I believe
the patch is correct.  However, it doesn't fix the bug completely for me (on Linux).
If I click directly on the "Write here" text, then it's fixed, but if I click in
the empty space below it, then it's not fixed.

Also, we need reftests for this.  Probably as subtests of
layout/base/tests/test_reftests_with_caret.html so we test with the caret visible.
I see at least four cases worth testing:
click on the text
click below the text
click on the text + a key event to insert a character
click below the text + a key event to insert a character
Also, the caret is rendered at the start of the line before the "Write here" text
which is a bit unfortunate since characters are actually added after it, but that
seems like a separate bug (the same thing occurs in Chrome, fwiw).  I think we
should render the caret after any ::before frames and before any ::after frames
because that's where inserted text will appear.
Still unfinished, but looks green and I'm about to board on an airplane: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4c235cceec58b002b56669cbf84041ac319ea48
Also, just for the record, I've verified that the two tests fail without my patch, but still left them in a separate commit so it's easier to verify.

And I've been browsing around and playing with editing apps just in case since this code doesn't look terribly well-tested, and everything looks sane.
Assignee: nobody → emilio
Component: DOM → Layout
FYI, I'm expecting an updated patch before reviewing this, since you said
it was unfinished in comment 24.
Flags: needinfo?(emilio)
(In reply to Mats Palmgren (:mats) from comment #26)
> FYI, I'm expecting an updated patch before reviewing this, since you said
> it was unfinished in comment 24.

Ouch, that "Still unfinished" was about the try run (when I posted it I was boarding an airplane, and it wasn't finished yet). This should be ready for review.

Sorry for the misunderstanding! :(
Flags: needinfo?(emilio)
Comment on attachment 8935836 [details]
Bug 1423331: Prevent selection from leaking from blocks.

https://reviewboard.mozilla.org/r/206710/#review216466

::: layout/generic/nsFrame.cpp:4919
(Diff revision 2)
>    nsBlockFrame::LineIterator firstLine = bf->LinesBegin();
>    nsBlockFrame::LineIterator end = bf->LinesEnd();
> -  if (firstLine == end) {
> -    nsIContent *blockContent = aFrame->GetContent();
> -    if (blockContent) {
> -      // Return with empty flag true.
> -      return FrameTarget(aFrame, false, false, true);
> -    }
> -    return FrameTarget::Null();
> -  }
>    nsBlockFrame::LineIterator curLine = firstLine;

nit: This is now the only use of 'firstLine', so I think we should remove it and initialize 'curLine = bf->LinesBegin()' instead.
Attachment #8935836 - Flags: review?(mats) → review+
Comment on attachment 8937334 [details]
Bug 1423331: Tests.

https://reviewboard.mozilla.org/r/208020/#review216468

::: layout/base/tests/bug1423331-1.html:20
(Diff revision 1)
> +  synthesizeMouseAtCenter(div, { type: "mousedown" });
> +  synthesizeMouseAtCenter(div, { type: "mouseup" });

With 'height:100px', I think these tests clicks below the "Write here" text.  Please add two more tests that are the same but clicks on the text, since we saw different behavior between those cases with the first patch.

Also, I think these two lines can be replaced with the more idiomatic:

  synthesizeMouseAtCenter(div, {});

and is there really a need to do it twice?
(It seems to me a single click would suffice for these tests.)
Attachment #8937334 - Flags: review?(mats) → review-
(FTR, I filed bug 1428643 for a similar bug with display:inline)
Comment on attachment 8937334 [details]
Bug 1423331: Tests.

https://reviewboard.mozilla.org/r/208020/#review216468

> With 'height:100px', I think these tests clicks below the "Write here" text.  Please add two more tests that are the same but clicks on the text, since we saw different behavior between those cases with the first patch.
> 
> Also, I think these two lines can be replaced with the more idiomatic:
> 
>   synthesizeMouseAtCenter(div, {});
> 
> and is there really a need to do it twice?
> (It seems to me a single click would suffice for these tests.)

I added the two other tests. The double-click is necessary to make the tests fail without the patch applied.
Comment on attachment 8937334 [details]
Bug 1423331: Tests.

https://reviewboard.mozilla.org/r/208020/#review216468

> I added the two other tests. The double-click is necessary to make the tests fail without the patch applied.

Oh, and thanks for the `synthesizeMouseAtCenter(div, {});`, didn't know about that one :)
Comment on attachment 8937334 [details]
Bug 1423331: Tests.

https://reviewboard.mozilla.org/r/208020/#review217078

(In reply to Emilio Cobos Álvarez [:emilio] from comment #33)
> I added the two other tests.

Thanks.

> The double-click is necessary to make the tests
> fail without the patch applied.

Seems a bit odd, but fair enough.
Attachment #8937334 - Flags: review?(mats) → review+
Backout by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/89d7ac0fbdc8
Backed out changeset b047f4782ae2 for 4 failures in layout/base/tests/test_reftests_with_caret.html r=backout on a CLOSED TREE
Ok, I'll take a look, the failures look like a bunch of them are in multi-range-user-select.html, and others are a timeout...

Oddly, it only happens in opt builds, linux debug runs it just fine.

I'll take a look over the week, but mats, given you're more familiar than me with these tests, any idea of where should I look at first? (No worries if you're busy)
Flags: needinfo?(mats)
Some of the failures say "iframe load for bug1123067-1.html timed out"
which is clearly due to enabling that test.  Perhaps the other failures
are also related to that somehow?  I would start by undoing that change
and submitting a Try run to see if the other failures still occur.
Flags: needinfo?(mats)
Huh, so this makes no sense. First here's a green try run with only the code changes (that is, only the first commit):

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3095b9b31cf2244b31dde9bf1829e36514d0c96

However, as soon as I add the tests (without uncommenting the other one), stuff goes nuts:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=92380ab810c972fd4ee7a48153462a03d21fdf1e&selectedJob=155556736

The tests seem a bit flaky on opt builds, guess I need to do an opt build to see if I can repro...
I couldn't repro, but I just noticed that most other testcases use reftest-wait, and my test-cases don't, d'oh!

Running a try run now.
Mats, are you fine with landing the last two tests commented out? I haven't been able to figure out why they mess up the other tests (as I write now, maybe it's because they share the reference with the first two? I'll give it another shot).

But I haven't been able to repro properly the failures locally, and I don't have much more cycles to debug it, I don't think it's worth to keep telegram broken.
Flags: needinfo?(emilio) → needinfo?(mats)
Would it possible to add the last two tests on OSX only?
(Try looks green on OSX with all tests)

Looks like there is a Linux-only section there already:
"if (navigator.platform.indexOf("Linux") >= 0)"
https://searchfox.org/mozilla-central/source/layout/base/tests/test_reftests_with_caret.html
so add something like that?
Flags: needinfo?(mats)
Otherwise, just comment them out for now is fine with me.  Please file
a follow-up bug in any case to enable those tests, with a reference in
test_reftests_with_caret.html so that someone in the future can figure
it out...
Blocks: 1434949
https://hg.mozilla.org/mozilla-central/rev/c41e1dcbc77a
https://hg.mozilla.org/mozilla-central/rev/0be927d6e79e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Is this something we should consider for uplift to Beta or can it ride the 60 train?
status-firefox-esr52: --- → unaffected
Flags: needinfo?(emilio)
Flags: in-testsuite+
I'd let it ride the trains, this is only a regression because I unshipped @-moz-document in beta / nightly, so release still works well. And as Mats said this code is... non-trivial.
Flags: needinfo?(emilio)
Regressions: 1571517
Regressions: 1698705
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: