[Bug 584632] Re: composer changes font mid email
Upstream report was closed "RESOLVED FIXED" on 2015-03-29 Unable to reproduce with Thunderbird 60.6 in Ubuntu 18.04 No comments here or upstream for 4 years so closing as fixed ** Changed in: thunderbird (Ubuntu) Status: Confirmed => Fix Released -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
The progress on squashing this bug is excellent. One thought is that currently TB v39 is in DAILY channel (where this fix has now presumably landed) and is due to move to EARLYBIRD week of March 31 which is tomorrow. Presumably if users are on the Earlybird channel then they will get the fix once v39 moves to that channel in the next few days? Of course it would mean a longer delay for those running stable versions. Anyway congratulations on this superb work. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
OK I guess the delay for one version is about two months if it does not get into v38? -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
No... if it doesn't get back-ported to 38, then it has to wait for version 45 - another, what, 10 months? -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Release channel is at 31.5 as of this post. Seems a long way to reach 39 assuming it moves +1 at a time -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
(In reply to maybe-the-one from comment #138) > Is there any way we can lobby for landing it in 38? Please move that discussion to bug 1142879 or elsewhere. Thanks! -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632] Re: composer changes font mid email
** Changed in: thunderbird Status: Confirmed => Fix Released -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Is there any way we can lobby for landing it in 38? -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
https://hg.mozilla.org/mozilla-central/rev/bf414f68291c -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Ummm... ok, but... who are these 'Thunderbird people', and how do we contact them? -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
OK, thanks for explaining. I did not know that there was shared code. And indeed, facts are facts even when one is ignorant of them. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Personally, I never understood why this problem involved browsers at all. It happens when COMPOSING email in THUNDERBIRD and manifests when reading email in THUNDERBIRD. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
(In reply to maybe-the-one from comment #135) > Personally, I never understood why this problem involved browsers at all. That you don't understand the facts doesn't change them ;-) Let me explain: Thunderbird is client software that sits on top of Mozilla core software. The so-called "editor" that is used in Thunderbird when writing an e-mail messase is Mozilla core software. It is also used in Firefox: Please go to http://www- archive.mozilla.org/editor/midasdemo/ to check that even in Firefox you can "write". The problem happens when losing the font goes unnoticed while writing. You will notice it when reading a reply that contains your original ill- formatted message. The (In reply to Charles from comment #134) > Ummm... ok, but... who are these 'Thunderbird people', and how do we contact > them? They are aware of the problem via this meta bug 1142879. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf414f68291c -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
For those who want to see this in Thunderbird 38 -- I suggest talking to the Thunderbird people and asking them if they can cherry-pick the patch for Thunderbird without affecting Firefox. If it's really a huge improvement for them, maybe they'll be willing to accept it despite lack of testing. Perhaps file a bug against the Thunderbird product, or get on IRC/e-mail/etc. with the appropriate people. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #125) > Charles, this patch affects more than Thunderbird. I'm not sure what the > usual practices are with regards to stuff that are regression prone in > Thunderbird, but in Firefox, we are usually very conservative, and prefer to > give things more time to bake. Yes, but Firefox is primarily a web browser - just how bad could a regression in some code in the editor be? That said, Jorg is correct that this isn't the right place for such a conversation... So, all that is left is to say THANK YOU!!! to Jorg, Ehsan, and everyone else involved for finally getting this bug squashed! -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
(In reply to Charles from comment #121) > Are you seriously saying that this bugfix will NOT land in time for TB 38? > Meaning, we will have to another full YEAR to see it fixed? Given that the version spacing is six weeks we get 6 * (45-38) = 42 weeks, that's 10 months ;-) However, everyone is free to compile their own version, which is what I'll do. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
I'm really sad that this patch will not land in TB 38. But I want to make a big thank to Jorg that is working on the correction of this long standing bug. Also thanks to Ehsan that worked this in the past. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
(In reply to Charles from comment #128) > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from > comment #125) > > Charles, this patch affects more than Thunderbird. I'm not sure what the > > usual practices are with regards to stuff that are regression prone in > > Thunderbird, but in Firefox, we are usually very conservative, and prefer to > > give things more time to bake. > > Yes, but Firefox is primarily a web browser - just how bad could a > regression in some code in the editor be? Potentially very bad. :-) Any regression that can break lots of websites is very bad. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Comment on attachment 8584608 Unified patch (code + test changes + three times revised new test) Review of attachment 8584608: - Can you please revise the commit message to explain what the patch does? Something like: "Bug 756984 - Collapse the selection on the last text node on the line, skipping br and inline frames when clicking past the end of line; r=roc,ehsan". Thanks again! Once you address these nits, this is ready to be checked in. ::: layout/generic/test/test_bug756984.html @@ +43,5 @@ > +is(selRange.endContainer.nodeName, "#text", "selection should be in > text node"); > +is(selRange.endOffset, 3, "offset should be 3"); > + } > + > + // click beyond the second line (100px to the left and 2px down), > expect DIV. Nit: please make this "first line". @@ +47,5 @@ > + // click beyond the second line (100px to the left and 2px down), > expect DIV. > + // This is the previous behaviour which hasn't changed since the line > is empty. > + // If the processing were wrong, the selection would end up in the > preceing non-empty line. > + theDiv = document.getElementById("div4"); > + sel = window.getSelection(); Nit: this is not needed. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Created attachment 8584639 Unified patch (code + test changes + four times revised new test) This should be good to go then ;-) -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Created attachment 8584608 Unified patch (code + test changes + three times revised new test) Here comes the updated test. This time with some additional inline elements. I'm not sure why the second test should be wrong, I'll add another attachment in a second to convince you ;-) -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Comment on attachment 8583354 Test case (revised) Review of attachment 8583354: - Looks like a great start! Did you intend to test the rest of the cases in a separate patch? ::: layout/generic/test/test_bug756984.html @@ +13,5 @@ > + > + href="https://bugzilla.mozilla.org/show_bug.cgi?id=756984";>Mozilla Bug > 756984 > + > + > + Since you mentioned you're new to writing tests, I'll add some bits about how this stuff works here. I hope that helps! The above is basically boilerplate that we include in most mochitests. The only parts that are really required are the two script elements that load the SimpleTest and event synthesis APIs. @@ +25,5 @@ > + > +/** Test for Bug 756984 **/ > +/** We test that clicking beyond the end of a line terminated with > selects the preceding text, if any **/ > + > +SimpleTest.waitForExplicitFinish(); This is used to tell the test harness to not stop the test once the page load finishes (which is the default.) This is required for all tests that can run past that point. For example, this test is asynchronous because of the waitForFocus call below. @@ +27,5 @@ > +/** We test that clicking beyond the end of a line terminated with > selects the preceding text, if any **/ > + > +SimpleTest.waitForExplicitFinish(); > + > +SimpleTest.waitForFocus(function() { This is required to ensure that the test window is raised to the top on the desktop, which is needed to ensure proper event delivery. @@ +35,5 @@ > + var sel = window.getSelection(); > + var f = document.getElementById("div1"); > + theDiv.focus(); > + sel.collapse(theDiv, 0); > + synthesizeMouse(theDiv, 100, 2, {type: "mousedown"}, f.contentWindow); The last argument of synthesizeMouse is the window object corresponding to the element that should receive the event. If omitted, the default is the current window object. Now, f is an HTMLDivElement, which doesn't have a contentWindow property, so f.contentWindow ends up as undefined. The reason this works is that from the perspective of the callee function, omitted arguments are undefined, so that's what this function checks for: <https://dxr.mozilla.org/mozilla- central/source/testing/mochitest/tests/SimpleTest/EventUtils.js#814>. So please just remove this last argument here and elsewhere in the test. @@ +36,5 @@ > + var f = document.getElementById("div1"); > + theDiv.focus(); > + sel.collapse(theDiv, 0); > + synthesizeMouse(theDiv, 100, 2, {type: "mousedown"}, f.contentWindow); > + synthesizeMouse(theDiv, 100, 2, {type: "mouseup"}, f.contentWindow); Nit: You can just issue one synthesizeMouse as {type: "click"} and it will dispatch both of these events internally. @@ +37,5 @@ > + theDiv.focus(); > + sel.collapse(theDiv, 0); > + synthesizeMouse(theDiv, 100, 2, {type: "mousedown"}, f.contentWindow); > + synthesizeMouse(theDiv, 100, 2, {type: "mouseup"}, f.contentWindow); > + var selObj = window.getSelection(); Nit: You already have |sel| above, no need to get it again, please remove this variable. @@ +49,5 @@ > + f = document.getElementById("div2"); > + theDiv.focus(); > + sel.collapse(theDiv, 0); > + synthesizeMouse(theDiv, 100, 2, {type: "mousedown"}, f.contentWindow); > + synthesizeMouse(theDiv, 100, 2, {type: "mouseup"}, f.contentWindow); Hmm, doesn't this also click to the right of the first line? @@ +53,5 @@ > + synthesizeMouse(theDiv, 100, 2, {type: "mouseup"}, f.contentWindow); > + selObj = window.getSelection(); > + selRange = selObj.getRangeAt(0); > + is(selRange.endContainer.nodeName, "DIV", "selection should be in > DIV"); > + is(selRange.endOffset, 0, "offset should be 0"); Not sure if I understand why this result would be desirable. I think that because you're clicking at the end of the first line, this is Gecko putting the selection at the end of the first line, which is on the first br element. Am I missing something? @@ +55,5 @@ > + selRange = selObj.getRangeAt(0); > + is(selRange.endContainer.nodeName, "DIV", "selection should be in > DIV"); > + is(selRange.endOffset, 0, "offset should be 0"); > + > + SimpleTest.finish(); This tells the test framework that the asynchronous test has been finished. This should be the last thing that the test does. If you have called waitForExplicitFinish in your test, it is an error to forget to call SimpleTest.finish(). -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
(In reply to Charles from comment #124) IMHO the problem is in the coupling between TB and FF. I think it's quite reasonable that the FF people are more conservative. Who would want to break a browser with a big market share. TB on the other hand is client software. An nothing should stop client software to pick its foundation and additional patches. I've seen client software (KompoZer, or for example KDE with its so-called "build dependencies") where I couldn't believe my eyes how much they patch the underlying components. This discussion, however, should be continued elsewhere. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Created attachment 8584620 Here comes another manual test. Here some HTML to run a test by hand. If clicking behind any of the eight lines in this test, "DIV" will be returned in the current version of FF. With the new behaviour clicking behind lines 1-6 and 8, "#text" will be returned, but for the empty line 7, DIV is still returned. If the code were wrong, this line couldn't be clicked at all and the caret would move to the previous or subsequent line. I know, because at first my code didn't cater for this case. IMHO my test is correct, if you don't think so, please be more specific. Changing the subject: I'd like to see this landed one day very soon. I'd also request to land it on mozilla38, so TB 38 can pick it up. I don't want to mention again that this is fixing a long-standing TB bug that people have been pulling their hair out for over 10 years now. If you really want more tests to be written, I can do that, but *afterwards* ;-) -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
(In reply to Jorg K from comment #118) > Created attachment 8584620 > Here comes another manual test. > > Here some HTML to run a test by hand. > > If clicking behind any of the eight lines in this test, "DIV" will be > returned in the current version of FF. > > With the new behaviour clicking behind lines 1-6 and 8, "#text" will be > returned, but for the empty line 7, DIV is still returned. > > If the code were wrong, this line couldn't be clicked at all and the caret > would move to the previous or subsequent line. I know, because at first my > code didn't cater for this case. > > IMHO my test is correct, if you don't think so, please be more specific. OK, I understand what you want to test now. So your test is actually correct, it was the comments where you mentioned clicking past the *second* line that confused me. :-) > Changing the subject: > I'd like to see this landed one day very soon. I'd also request to land it > on mozilla38, so TB 38 can pick it up. I don't want to mention again that > this is fixing a long-standing TB bug that people have been pulling their > hair out for over 10 years now. Sorry but this patch is not suitable for backporting into Aurora (which will soon become Beta) at this point, because it's quite risky in terms of the possibility to cause regressions. It needs to land on trunk and ride the trains. I understand that you'd like to see it fixed as soon as possible, but please be a bit more patient. :-) > If you really want more tests to be written, I can do that, but *afterwards* > ;-) OK, that's fair. :-) -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #119) > (In reply to Jorg K from comment #118) >> Changing the subject: >> I'd like to see this landed one day very soon. I'd also request to land it >> on mozilla38, so TB 38 can pick it up. I don't want to mention again that >> this is fixing a long-standing TB bug that people have been pulling their >> hair out for over 10 years now. > Sorry but this patch is not suitable for backporting into Aurora (which will > soon become Beta) at this point, because it's quite risky in terms of the > possibility to cause regressions. It needs to land on trunk and ride the > trains. I understand that you'd like to see it fixed as soon as possible, > but please be a bit more patient. :-) Oh god, I hope I'm mis-reading this... Are you seriously saying that this bugfix will NOT land in time for TB 38? Meaning, we will have to another full YEAR to see it fixed? Please tell me it ain't so. As for regressions - so you're saying it is OK for major regressions to happen for the entire lifecycle of TB 31 (see all of the Address auto- entry bugs that are only just now being fixed), but not for Firefox? -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
(In reply to Jorg K from comment #123) > (In reply to Charles from comment #121) > > > Are you seriously saying that this bugfix will NOT land in time for TB 38? > > Meaning, we will have to another full YEAR to see it fixed? > > Given that the version spacing is six weeks we get 6 * (45-38) = 42 weeks, > that's 10 months ;-) > However, everyone is free to compile their own version, which is what I'll > do. Fine for us, but what about companies using TBird... I just don't get this kind of reluctance... even if there were regressions, they could easily be fixed... Oh well, I guess beggars cannot be choosers... -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Created attachment 8584346 Unified patch (code + test changes + twice revised new test) Thank you very much for the review and all the additional explanation. I hope I could address your questions in additional comments in the test. Sadly switching from "mousedown/up" to "click" as you had suggested makes the test fail with: failed | uncaught exception - NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowUtils.sendMouseEvent] at chrome://specialpowers/content/specialpowersAPI.js:161 This is actually the only test I intended to submit ;-) This is for the case where the user clicks beyond the end of the line. There are two tests already which I had to adapt to the new selection behaviour which test the moving to the end (editor/libeditor/tests/test_selection_move_commands.xul) and moving from the subsequent line using left arrow (layout/generic/test/test_movement_by_characters.html). If you wanted to be really purist a test with a "div contenteditable" could be added where an empty line is first filled with some text, which is then removed so the range becomes empty. The code caters for this case. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Charles, this patch affects more than Thunderbird. I'm not sure what the usual practices are with regards to stuff that are regression prone in Thunderbird, but in Firefox, we are usually very conservative, and prefer to give things more time to bake. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
(In reply to Jorg K from comment #115) > Created attachment 8584346 > Unified patch (code + test changes + twice revised new test) > > Thank you very much for the review and all the additional explanation. > I hope I could address your questions in additional comments in the test. > > Sadly switching from "mousedown/up" to "click" as you had suggested makes > the test fail with: > failed | uncaught exception - NS_ERROR_FAILURE: Component returned failure > code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMWindowUtils.sendMouseEvent] at > chrome://specialpowers/content/specialpowersAPI.js:161 Err, sorry, I misspoke. I should have said: "remove the type value" altogether (just pass an empty object `{}'). That should do the right thing: <https://dxr.mozilla.org/mozilla- central/source/testing/mochitest/tests/SimpleTest/EventUtils.js#272> > This is actually the only test I intended to submit ;-) > > This is for the case where the user clicks beyond the end of the line. > There are two tests already which I had to adapt to the new selection > behaviour which test the moving to the end > (editor/libeditor/tests/test_selection_move_commands.xul) > and moving from the subsequent line using left arrow > (layout/generic/test/test_movement_by_characters.html). > > If you wanted to be really purist a test with a "div contenteditable" could > be added where an empty line is first filled with some text, which is then > removed so the range becomes empty. The code caters for this case. I think relying on the selection movement tests is sort of OK, but I really prefer to have much better testing on this, since this code is extremely fragile and I'm pretty sure that without extensive tests, we'll end up breaking it again in the future. That being said, I guess it's not fair for me to make you do this. :-) But at the very least, we should have tests for clicking past the end of the line with the line terminating with a bunch of inline elements that have a text node inside them. That is essentially what this bug is really about. So please add some test along those lines. Also, to reiterate, the second test in your test case is wrong... -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Comment on attachment 8584639 Unified patch (code + test changes + four times revised new test) Review of attachment 8584639: - Thanks, looks great! -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
(In reply to Jorg K from comment #110) > Where in the mochitest.ini do I put my new test? I put it right at the front > since I don't understand the syntax of "skip-if". Does that skip the next > line? Perhaps you can suggest a line where it should go. Or say: After such > and such. Just add a line that says [test_bug756984.html] (assuming that's the file's name). Put it right before the next test line, like "[test_bug784410.html]" or whatever. The "skip-if", "support-files", etc. lines apply to the preceding test file, so don't put it before one of those lines. (This is confusing. I looked it up in the online docs: https://ci.mozilla.org/job/mozilla-central-docs/Tree_Documentation/build/buildsystem/test_manifests.html) > In the test I use a 123 to get offset 3 when clicking behind > it. Without the span, I get offset 4, which I find surprising, or clicking > at the front gives offset 1 instead of 0. FF 36 does the same, so I guess > it's right, but I'd like to understand why 4 and not 3, or 1 and not 0. Why > does the span make a difference? You have a newline at the beginning of the div. That's the first character in the text node (although it doesn't normally render). If you did 123456 all on one line, the offsets would be as you expected even without the span. > Anyway, the tests are passed. Without my changes, the first test fails as > expected. > > As I said in comment #106: There are already tests for hitting the "end" key > and navigating with the arrow keys, so this test should be the only one we > need additionally. Sounds great! -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
I'll revise the test and post a new one in a second. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Created attachment 8583354 Test case (revised) If this is good to go, I'll submit another "unified" patch (code + all tests), if necessary. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Created attachment 8583259 Test case Here is the test case. Forgive me if the JS is bad, I've never written any, just copied bits and pieces around. Two questions: Where in the mochitest.ini do I put my new test? I put it right at the front since I don't understand the syntax of "skip-if". Does that skip the next line? Perhaps you can suggest a line where it should go. Or say: After such and such. In the test I use a 123 to get offset 3 when clicking behind it. Without the span, I get offset 4, which I find surprising, or clicking at the front gives offset 1 instead of 0. FF 36 does the same, so I guess it's right, but I'd like to understand why 4 and not 3, or 1 and not 0. Why does the span make a difference? Anyway, the tests are passed. Without my changes, the first test fails as expected. As I said in comment #106: There are already tests for hitting the "end" key and navigating with the arrow keys, so this test should be the only one we need additionally. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Try running the tests locally without your patches if you want to be sure (hg qpop -a will get rid of them if you're using mq). If they fail even without your patches, don't worry about it. In theory all tests should work on all supported platforms and configurations, but in practice some small fraction will break on some systems for various reasons, and in practice we only require that they work on the try servers. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Created attachment 8582701 Easy fix for test due to new selection behaviour (richtext2) Removed tests which now no longer fail due to changed selection behaviour. I'd appreciate some help with the stuff from comment #97: How do I run test_selection_move_commands.xul separately and what do I make of the additional unexpected failures? -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Comment on attachment 8582701 Easy fix for test due to new selection behaviour (richtext2) Review of attachment 8582701: - Fantastic! r=me. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
(In reply to Jorg K from comment #97) > These tests are a little mysterious. I wanted to look at > editor/libeditor/tests/test_selection_move_commands.xul first. Sadly > mach mochitest-plain editor/libeditor/tests/test_selection_move_commands.xul > doesn't work? I've run single tests before, but they weren't XUL files. That test is part of the mochitest-chrome test suite, so you can use either mach mochitest-chrome, or even better, mach mochitest (which figures out which mochitest variant a test lives in.) (The way you can know what test suite a test file belongs to is to look for its name in the .ini files in the same directory, for example, mochitest.ini and chrome.ini for mochitest-plain and mochitest-chrome, respectively.) -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Is it worth running it on other systems as well since I found some - most likely unrelated - problems (see comment #97) when running it locally on Windows 7? Or are these known problems that always fail? -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Created attachment 8582633 Patch to fix all three problems: Click, "end" key, left arrow from start of previous line (updated coding style) Here's the updated code according to the review. I will review the test failures next. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
(In reply to :Aryeh Gregor from comment #84) > A good try line to use by default for editor changes is: > try: -b do -p all -u all[x64] -t none OK, but how do I send the patch to the try server? I have my "level 1" access rights and I believe SSH is set up correctly. I tried hg push -f ssh://mozi...@jorgk.com@hg.mozilla.org/try/ *before* coming across the "hg qnew" command. It returned: "No changes found" or words to that extent. I haven't tried after the "hg qnew". So what is the exact sequence of commands? BTW, I'm on Windows 7. 30 seconds of your time save me three hours of (very frustrating) research (since I want to focus on the problem and not the infrastructure). As for the test results: Mochitests 4, 5 and oth failed. I just had a very brief look: oth says: editor/libeditor/tests/test_selection_move_commands.xul | node after cmd_selectEndLine - got [object Text], expected [object HTMLBodyElement] YES! Indeed, we want the text. I will need to change the test. 5 says: layout/generic/test/test_movement_by_characters.html | Left movement broken in H K - got [object Text], expected [object HTMLDivElement] Same thing, we want the text. 4 says: TEST-UNEXPECTED-PASS a few times, so let's how that's a change for the better. However, this stuff worries me (3x crashed, 1x time out) 965721 Intermittent test_bug409604.html, test_bug719533.html, test_richtext2.html, test_bug412567.html | application crashed [@ imgStatusTracker::RecordCancel()] 969526 Intermittent test_richtext.html,test_richtext2.html,test_bug436801.html | application crashed [@ KERNELBASE.dll + 0x89ae4] (ABORT: Should have mProgressTracker until we create mImage: 'mProgressTracker') 1129538 Intermittent test_draggableprop.html,test_richtext2.html | application crashed [@ mozalloc_abort(char const*)] after "ABORT: Should have given mProgressTracker to mImage: '!mProgressTracker', file /image/src/imgRequest.cpp, line 149" 1142900 Intermittent test_richtext2.html | application timed out after 330 seconds with no output Any comments on these crashes and time-outs? P.S.: Would you be able to let me know your time-zone so I know when not to expect feedback ;-) -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Comment on attachment 8582686 Easy fix for test due to new selection behaviour (layout/generic) This fixes the failed test in layout/generic. More to come. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
These tests are a little mysterious. I wanted to look at editor/libeditor/tests/test_selection_move_commands.xul first. Sadly mach mochitest-plain editor/libeditor/tests/test_selection_move_commands.xul doesn't work? I've run single tests before, but they weren't XUL files. So I ran mach mochitest-plain editor/libeditor/tests/ and got: 7043 INFO TEST-UNEXPECTED-PASS | editor/libeditor/tests/browserscope/test_richte xt2.html | Browserscope richtext2 selection: S-Proposed-SM:e.f.lb_BR.BR-1_SC-1-d M - 1 should equal 1 TEST-INFO | expected FAIL 7044 INFO TEST-UNEXPECTED-PASS | editor/libeditor/tests/browserscope/test_richte xt2.html | Browserscope richtext2 selection: S-Proposed-SM:e.f.lb_BR.BR-1_SC-1-b ody - 1 should equal 1 TEST-INFO | expected FAIL 7045 INFO TEST-UNEXPECTED-PASS | editor/libeditor/tests/browserscope/test_richte xt2.html | Browserscope richtext2 selection: S-Proposed-SM:e.f.lb_BR.BR-1_SC-1-d iv - 1 should equal 1 TEST-INFO | expected FAIL 7046 INFO TEST-UNEXPECTED-PASS | editor/libeditor/tests/browserscope/test_richte xt2.html | Browserscope richtext2 selection: S-Proposed-SM:e.f.lb_BR.BR-1_SM-1-d M - 1 should equal 1 TEST-INFO | expected FAIL 7047 INFO TEST-UNEXPECTED-PASS | editor/libeditor/tests/browserscope/test_richte xt2.html | Browserscope richtext2 selection: S-Proposed-SM:e.f.lb_BR.BR-1_SM-1-b ody - 1 should equal 1 TEST-INFO | expected FAIL 7048 INFO TEST-UNEXPECTED-PASS | editor/libeditor/tests/browserscope/test_richte xt2.html | Browserscope richtext2 selection: S-Proposed-SM:e.f.lb_BR.BR-1_SM-1-d iv - 1 should equal 1 TEST-INFO | expected FAIL 7049 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug1068979.html | S hould have four SMP characters - got 𝐀𝐀, expected 𝐀𝐀𝐀𝐀 7050 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug1068979.html | T hree complete characters should remain - got 𝐀, expected 𝐀𝐀𝐀 7051 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug1068979.html | T wo complete characters should remain - got , expected 𝐀𝐀 7052 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug1068979.html | O nly one complete SMP character should remain - got , expected 𝐀 7053 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug410986.html | Co ntent should be pasted in plaintext format - got , expected green text 7054 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug410986.html | Ti med out while polling clipboard for pasted data. - expected PASS 7055 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug410986.html | Fa iled to copy the second item to the clipboard - expected PASS 7056 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug551704.html | Ti med out while polling clipboard for pasted data. - expected PASS SUITE-END | took 376s So we got a few TEST-UNEXPECTED-PASS, we already new that. No sight of test_selection_move_commands.xul, but there are other failures that don't appear in https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd11f71e3daa Yes, I'm running the text on Windows 7, not on Linux, but still. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Thanks, the "mach mochitest-chrome" worked, I will supply a fixed test tomorrow (now after 12 AM here). I assume I can just push a "unified" patch with the code and the updated tests to the try server. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
In .hg/hgrc (or ~/.hgrc) set up the path alias: mc-try = ssh://mozi...@jorgk.com@hg.mozilla.org/try Then hg qgoto your-patch hg qnew -m "try: -b do -p all -u all[x64] -t none" try hg push -f -r tip mc-try && hg qpop && hg qdelete try Or at least that what I do. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Created attachment 8582686 Easy fix for test due to new selection behaviour (layout/generic) -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #88) > Nit: you don't need to mention bug numbers in comments. This information is > available through hg/git blame. There are many bug numbers in the code (including some that you entered for bug 596506), so this policy must have changed recently. > Also, please wrap if bodies in braces. Again, the policy seems to have changed here since there are many conditions of this style: if (!range.content) return NS_ERROR_FAILURE; > There is already code which gets the line frame count earlier in this > function. Please refactor it out of the else block it currently lives in > and just use lineFrameCount from that code here. That already takes care of > checking the return value of GetLine for errors too. Also, I think you want > to check atLineEdge instead of aJumpLines here. I disagree. I did what you suggest, but it didn't work. The it->GetLine(thisLine, &firstFrame, &lineFrameCount, nonUsedRect); ealier on gets the details for the previous line where the left arrow was pressed. We then move on to the line before. The next it->GetLine(currentLine, ¤tFirstFrame, &lineFrameCount, usedRect); is operating on the line we've just moved to, the one where we want to skip the brFrame. The only thing which would be a bit cleaner is not to re-use the "it" variable but to declare another one. I agree, I do need "atLineEdge". -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Created attachment 8582923 Easy fix for test due to new selection behaviour (move commands) This is the last of the updated tests. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
(In reply to Jorg K from comment #92) > OK, but how do I send the patch to the try server? I have my "level 1" > access rights and I believe SSH is set up correctly. I tried > hg push -f ssh://mozi...@jorgk.com@hg.mozilla.org/try/ *before* coming > across the "hg qnew" command. > It returned: "No changes found" or words to that extent. I haven't tried > after the "hg qnew". > So what is the exact sequence of commands? BTW, I'm on Windows 7. 30 seconds > of your time save me three hours of (very frustrating) research (since I > want to focus on the problem and not the infrastructure). If you have some changes you want to commit, and no patches currently applied: hg qnew mypatchname # This saves your changes to an mq patch hg qnew -m "try: -b do -p all -u all[x64] -t none" try # Make a second patch with the try line hg push -f mc-try # Make sure you have Magnus' line in .hg/hgrc hg qdelete try # Delete the empty try patch Note: this will leave you with a patch in mq called "mypatchname" with your changes. If you make any new changes to the patch and want to submit to try again, do the same, except instead of the first line, do "hg qref" to refresh the existing patch instead of making a new one. To view your existing patches, try "hg qser -s"; to move around, you can use "hg qpop" and "hg qpush" and "hg qgoto"; for more help, try "hg help mq" or "hg help commandname". If you see any weird changes that only appear on some platforms and don't look related to your changes, they're probably random (intermittent) failures that are unrelated to your changes. You can usually spot changes that you really caused because they'll show up on all platforms, and look related to your changes. If you're not sure, you can ask us, or ask on IRC for a quicker response. > However, this stuff worries me (3x crashed, 1x time out) > 965721 Intermittent test_bug409604.html, test_bug719533.html, > test_richtext2.html, test_bug412567.html | application crashed [@ > imgStatusTracker::RecordCancel()] > 969526 Intermittent > test_richtext.html,test_richtext2.html,test_bug436801.html | application > crashed [@ KERNELBASE.dll + 0x89ae4] (ABORT: Should have mProgressTracker > until we create mImage: 'mProgressTracker') > 1129538 Intermittent test_draggableprop.html,test_richtext2.html | > application crashed [@ mozalloc_abort(char const*)] after "ABORT: Should > have given mProgressTracker to mImage: '!mProgressTracker', file > /image/src/imgRequest.cpp, line 149" > 1142900 Intermittent test_richtext2.html | application timed out after 330 > seconds with no output > > Any comments on these crashes and time-outs? Don't worry about those. These tests fail sometimes at random -- it's not connected to your changes. > P.S.: Would you be able to let me know your time-zone so I know when not to > expect feedback ;-) I'm UTC+0200, and switching to UTC+0300 this Thursday night. Last I was aware, Ehsan lives in eastern Canada and so should be UTC-0400. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Comment on attachment 8582633 Patch to fix all three problems: Click, "end" key, left arrow from start of previous line (updated coding style) Review of attachment 8582633: - This looks fine to me, and in fact I think it's ready for roc to review! -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #87) > I realized that I forgot about another case that we need to test. br frames > are not the only reason for a line ending, we can also get line breaks at > block boundaries, for example: block 1new line > hereblock2 This works already. My patch only tries to skip brFrames. In this example there is no brFrame at the end of "new line here". -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
(In reply to Jorg K from comment #106) > Pushed to try server (thanks guys for the support!): > https://treeherder.mozilla.org/#/jobs?repo=try&revision=9782fa678cd1 Treeherder isn't loading for me right now, but someone else who looked said it seemed fine. > Just out of interest: Why build on all platforms and then just execute on > Linux? I mean, why build it in the first place? Just to see that it > compiles? If it already compiled once, and I only rerun tests, that doesn't > make too much sense. Yes, to see that it compiles. Different platforms use different compilers, and maybe sometimes different compiler versions, and they treat different things as errors. Compiling the code on all platforms is usually a good resources-safety tradeoff. (Ideally all tests would be run on all platforms, but it overloads the try servers.) > Further steps: If this try run goes well, the only thing missing is a test > for the changes I made. Two scenarios are already covered by the existing > tests which I had to change: the "moving around with the left arrow" and the > "jumping to the end of the line with a key stroke". What's missing is the > "clicking beyond the end of the line". The first try run in comment #71 was > done with only the code to fix the clicking. No tests failed, so there > wasn't a test for this, so it needs to be added now. Sounds good! -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
(In reply to Jorg K from comment #90) > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from > comment #88) > > Nit: you don't need to mention bug numbers in comments. This information is > > available through hg/git blame. > There are many bug numbers in the code (including some that you entered for > bug 596506), so this policy must have changed recently. Well you're repeating it multiple times in the same file. The bug number is really not adding any useful information in this case. > > Also, please wrap if bodies in braces. > Again, the policy seems to have changed here since there are many conditions > of this style: > if (!range.content) > return NS_ERROR_FAILURE; We unfortunately don't consistently adhere to the coding style, but we should for new code that we're adding. > > There is already code which gets the line frame count earlier in this > > function. Please refactor it out of the else block it currently lives in > > and just use lineFrameCount from that code here. That already takes care of > > checking the return value of GetLine for errors too. Also, I think you want > > to check atLineEdge instead of aJumpLines here. > I disagree. I did what you suggest, but it didn't work. The > it->GetLine(thisLine, &firstFrame, &lineFrameCount, nonUsedRect); > ealier on gets the details for the previous line where the left arrow was > pressed. > We then move on to the line before. The next > it->GetLine(currentLine, ¤tFirstFrame, &lineFrameCount, usedRect); > is operating on the line we've just moved to, the one where we want to skip > the brFrame. > The only thing which would be a bit cleaner is not to re-use the "it" > variable but to declare another one. Oh yeah, good point. So please make sure you check the return value of GetLine like above. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Created attachment 8582931 Unified patch (code + test changes) ready for next push to try. Pushed to try server (thanks guys for the support!): https://treeherder.mozilla.org/#/jobs?repo=try&revision=9782fa678cd1 Aryeh: Please help me review the results. Actually, I followed what was suggested for the try: try: -b do -p all -u all[x64] -t none Just out of interest: Why build on all platforms and then just execute on Linux? I mean, why build it in the first place? Just to see that it compiles? If it already compiled once, and I only rerun tests, that doesn't make too much sense. Further steps: If this try run goes well, the only thing missing is a test for the changes I made. Two scenarios are already covered by the existing tests which I had to change: the "moving around with the left arrow" and the "jumping to the end of the line with a key stroke". What's missing is the "clicking beyond the end of the line". The first try run in comment #71 was done with only the code to fix the clicking. No tests failed, so there wasn't a test for this, so it needs to be added now. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Comment on attachment 8582360 Patch to fix all three problems: Click, "end" key, left arrow from start of previous line Review of attachment 8582360: - Some feedback on your patch so far. ::: layout/generic/nsFrame.cpp @@ +3555,3 @@ >for (int32_t n = aLine->GetChildCount(); n; > --n, frame = frame->GetNextSibling()) { > +// Bug 756984: Skip brFrames. Can only skip if the line contains at > least one selectable and non-empty frame before Nit: you don't need to mention bug numbers in comments. This information is available through hg/git blame. @@ +3555,4 @@ >for (int32_t n = aLine->GetChildCount(); n; > --n, frame = frame->GetNextSibling()) { > +// Bug 756984: Skip brFrames. Can only skip if the line contains at > least one selectable and non-empty frame before > +if (!SelfIsSelectable(frame, aFlags) || frame->IsEmpty() || (canSkipBr > && frame->GetType() == nsGkAtoms::brFrame)) Nit: please wrap lines at 80 characters. For example, according to our coding style, this condition should be written as: if (!SelfIsSelectable(frame, aFlags) || frame->IsEmpty() || (canSkipBr && frame->GetType() == nsGkAtoms::brFrame)) @@ +6801,5 @@ > for (int32_t count = lineFrameCount; count; > --count, frame = frame->GetNextSibling()) { >if (!frame->IsGeneratedContentFrame()) { > +// Bug 756984: When jumping to the end of the line with the > "end" key, skip over brFrames > +if (endOfLine && lineFrameCount > 1 && frame->GetType() == > nsGkAtoms::brFrame) continue; Nit: again, please adjust the whitespace here. Also, please wrap if bodies in braces. @@ +7090,5 @@ > + nsIFrame *currentBlockFrame, *currentFirstFrame; > + nsRect usedRect; > + int32_t currentLine = nsFrame::GetLineNumber(traversedFrame, > aScrollViewStop, ¤tBlockFrame); > + nsAutoLineIterator it = currentBlockFrame->GetLineIterator(); > + it->GetLine(currentLine, ¤tFirstFrame, &lineFrameCount, > usedRect); There is already code which gets the line frame count earlier in this function. Please refactor it out of the else block it currently lives in and just use lineFrameCount from that code here. That already takes care of checking the return value of GetLine for errors too. Also, I think you want to check atLineEdge instead of aJumpLines here. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Looks like Aryeh answered your questions (thanks Aryeh!) I realized that I forgot about another case that we need to test. br frames are not the only reason for a line ending, we can also get line breaks at block boundaries, for example: block 1new line hereblock2 We need to ensure that the selection ends up on the text node when you put the selection at the end of "new line here" using any of the methods mentioned before. Again, please note that some of those cases may already work properly and/or be fixed by your patch, but it's worth testing. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
I try not to do this very often anymore (useless comments in bugs), but... I just wanted to say THANK YOU to Jorg, Ehsan and Aryeh (and anyone else involved) working on this bug!! I must say, after the scare announcement from Mozilla about not directly funding Thunderbird anymore, I was worried, but it seems that there is much more movement on lots of older bugs (and new ones) lately, and it is extremely gratifying to see people stepping up. I just wish I had some coding skills (and time to use them)... So - thanks to all!!! -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
The failure in editor/libeditor/tests/test_selection_move_commands.xul also needs looking at. Otherwise, looks good! -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Created attachment 8582014 four line change to fix a 10 y/o problem ;-) Here's a new patch. This fixes click beyond end of line *and* "end" key. Still open: Left arrow at the start of the subsequent line. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Created attachment 8582360 Patch to fix all three problems: Click, "end" key, left arrow from start of previous line Can you please push this to the try server for me and tell me the exact steps to do it. I was reading https://wiki.mozilla.org/ReleaseEngineering/TryServer#How_to_push_to_try and http://trychooser.pub.build.mozilla.org/ I don't know which tests we need to run here and on which platforms. I'm also struggling with Mercurial. This https://developer.mozilla.org/en/docs/Mercurial_FAQ apparently is out of date. I got to the "hg qnew", the patch is enclosed. I tried "hg commit" but got "abort: cannot commit over an applied mq patch". Hmm. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
(In reply to :Aryeh Gregor from comment #77) > For the record, from black-box testing of WebKit a few years ago, it looked > like it normalized the selection after every change. Even if you called > .addRange(), it copied the range and then stuck the selection endpoints > inside a nearby text node if available, etc. I think it's taking things too > far to change script-specified selections Yeah, I think that's a bit too aggressive, and probably would cause authors to have to do insane hacks if they actually want to put the selection where the engine doesn't think is appropriate. > but the right way to do this is > probably to have some sort of helper method in Selection like > NormalizePoint(nsINode*, int32_t) and call it before every user-initiated > selection change. We might want to disallow other types of user-created > selections from occurring in the future, although my brain is too rusty to > supply any. Well, most of the code handling selection changes actually lives outside Selection. ;-) Depending on where we need to modify the behavior, having this kind of helper may or may not help... See Jorg's patch for example, I think the way he modifies the existing logic is cleaner than going with the current logic and then fixing it up elsewhere. > Do we want to allow a selection like foo{}bar, with the > selection collapsed in between the and ? IIRC, WebKit in my testing > forced this to be foo[]bar (always making it on the previous > text node). A ten-second test in WordPad suggests this is the right thing > to do. Yes, that makes sense to me. Follow-up bug perhaps? -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
BTW, Jorg, since your fix at least improves the situation where someone clicks at the end of the line, I'd be fine with landing it with a test case in this bug if you prefer to move the rest of the investigation and the fixes into another bug. Whichever way you prefer is fine. :-) -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
A good try line to use by default for editor changes is: try: -b do -p all -u all[x64] -t none This will build on all platforms (to detect compile errors), and will run tests only on 64-bit Linux (to avoid wasting resources). If there might be platform-specific test failures, remove the "[x64]", but I don't think you need to do that here. For Mercurial -- if you're using mq (which you should), you don't ever want to do "hg commit". The patch you've attached is exactly right. Here are the try results for you: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd11f71e3daa -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Okay, this caused some try failures. One of them is an unexpected pass in richtext2, which is good! It means you just have to update the test so it knows we're supposed to pass now. In the case of richtext2, you want to edit the file editor/libeditor/tests/browserscope/lib/richtext2/currentStatus.js and remove the appropriate lines (this is different from how most tests need to be updated). To check that it worked, run this (which may take a while and you have to keep the browser window focused): ./mach mochitest-plain editor/libeditor/tests/browserscope If you're on Linux, you should be able to install the appropriate package for the xvfb-run command and use this instead so it runs without creating a window you have to keep in focus: xvfb-run ./mach mochitest-plain editor/libeditor/tests/browserscope The other failure I see is layout/generic/test/test_movement_by_characters.html, which you need to look at. It could be your patch has a problem, or it could be the test needs to be updated. If you're having trouble, please feel free to ask for help! -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Thanks for the suggestions made in comment #78. I'm testing with foo bar on the same line foo bar on the same line foo bar on the same line This is strikethrough 25 m2 H2SO4 and some underline foobar Moving around with the cursor already works with the current version of FF, including moving from a long line to a short one and getting to the end this way. Pressing the "End" button is sadly NOT fixed by the patch. Clicking at the start of a line and then pressing "left arrow" to get to the end of the previous line is also NOT fixed by the patch. So the next step is to fix the two mentioned cases: End button and "left arrow" at the start of the line. BTW, these two cases do work in Chrome and IE already. I'd like to make sure that all positioning actions end up with the same correct result, clicking is only one of them. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Cool, so yeah, a fix to those two additional cases + the unit tests is probably all that we'd need here! -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Sorry for the delay here, somehow I failed to note the needinfo flag! As Aryeh said, the current try results look great! And it seems like fixing this is going to be much easier than I thought after all. :-) The next step is to ensure that the selection is put in the exact same place through other means of moving to the end of a line. For example, one such scenario that you need to investigate is if the caret is in the middle of the line, and you press End or Ctrl/Cmd+E. Another example is when you have multiple lines, and you place the caret at the end of a longer line and use up/down to navigate to the end of a shorter line. Same thing with using PageUp/PageDown in the same situation but with more lines of text obviously. Some of these code paths can end up going through the same code and some may already do the right thing but you need to test each one of them to make sure. The main entry point for all of these is nsFrameSelection::MoveCaret, where we end up calling nsIFrame::PeekOffset <https://dxr.mozilla.org/mozilla- central/source/layout/generic/nsSelection.cpp#969> which then gets us through the same machinery as you've seen before. Another thing that would be useful to test (but I'm pretty sure that it would work correctly based on the code changes) is what happens in both of these cases when you have several inline elements nested underneath each other, such as: foo bar on the same line You should ensure that the selection is again put at the end of the text frame in this case, so that when the user starts to type again, the new text would have the correct styles imposed by all of those inline elements. Once you've verified and fixed all of these cases, the next step would be to write a unit test as Aryeh mentioned that examines this behavior and ensures that we don't end up regressing it in the future. In addition to simulating a mouse click, you'd need to use synthesizeKey, for example, synthesizeKey("VK_UP") for simulating the "Up" key, and so on. And thanks again for working on this! -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
For the record, from black-box testing of WebKit a few years ago, it looked like it normalized the selection after every change. Even if you called .addRange(), it copied the range and then stuck the selection endpoints inside a nearby text node if available, etc. I think it's taking things too far to change script-specified selections, but the right way to do this is probably to have some sort of helper method in Selection like NormalizePoint(nsINode*, int32_t) and call it before every user-initiated selection change. We might want to disallow other types of user-created selections from occurring in the future, although my brain is too rusty to supply any. Do we want to allow a selection like foo{}bar, with the selection collapsed in between the and ? IIRC, WebKit in my testing forced this to be foo[]bar (always making it on the previous text node). A ten-second test in WordPad suggests this is the right thing to do. I don't think any of this has to be in the scope of the current bug, though. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Oops, I missed that part of comment #68 -- should have read more carefully. I don't know what more there is to do, since it passed a try run. But that's why Ehsan is in charge and not me. ;) In any event, you will need a test at the end of the day, so you could still go ahead and write it now. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
The try results look good to me, so you just need to include an automated regression test (mochitest) and you can ask a reviewer to approve it to be included in Firefox. You want to add a file patterned off something like this: <https://dxr.mozilla.org/mozilla- central/source/layout/generic/test/test_bug970363.html> You could base it off one of the tests you attached to this bug, but instead of asking the user to click, you have to synthesize a click using a function from EventUtils.js <https://dxr.mozilla.org/mozilla- central/source/testing/mochitest/tests/SimpleTest/EventUtils.js>, possibly synthesizeMouse. And instead of doing alert(), do something like is(myVariable, "#text", "selection should be in text node"). You have to add your test's name to the file layout/generic/test/mochitest.ini, and then you can run it with "./mach mochitest-plain layout/generic/test/test_bug756984.html". It should fail before the patch is applied, and pass after the patch is applied. Thanks a ton for working on this! If you want feedback from me on if your test looks good before asking for review, please feel free to ask me via the "feedback" flag when uploading your patch. I'm probably less busy than the reviewers. :) -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
@Aryeh: Yor comment really surprises me given the following: In comment #68 Ehsan said: > Good start, but this is still *far* from being ready for review. In comment #60 Ehsan said: > This affects more than just Thunderbird, and concerns behavior that is > visible to Web pages. > Web content can rely on the behavior of the engine in a lot of ways, for > example by expecting > that Firefox puts the selection somewhere specific when you go to the end of > line. > This is one of the reasons why fixing this bug is *very difficult*. Also, we need to keep the sister bug 1140617 in mind. In bug 1140617 comment #8 Ehsan said: > We still don't have a complete fix for bug 756984. The stuff that you're > working on is > just the tip of the *iceberg*. Can you please just *trust me* on this and > wait for bug > 756984 to be finished before worrying about this? Thanks! To me, it's just a three line change, and if you want, with a test case. Obviously none of the other tests checks the selection after the click, so I can certainly add such a test following your suggestions. However, before I start, I want to make 100% sure that we're all on the same page. I have the impression that Ehsan has something grander in mind. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
When you have a minute, could you please look at the results or instruct me, how to interpret them. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Pushed to Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb676357b6c9 -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Sorry, you need to help me out here. Never done it before. I will get the access rights as discussed on IRC. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Comment on attachment 8576888 three line change to fix a 10 y/o problem ;-) Review of attachment 8576888: - Good start, but this is still far from being ready for review. Did you push this to the try server? Please include the treeherder URL so that we can look at the test failures. Thanks! -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
(In reply to Archaeopteryx [:aryx] from comment #70) > Pushed to Try: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb676357b6c9 Canceled this as it misses the most important part of the tests, mochitests. Repushed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b34acaf40c3 -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Created attachment 8576888 three line change to fix a 10 y/o problem ;-) Skipping brFrames when determining the closest frame in a line. Can only skip if the brFrame isn't the only selectable non-empty frame in the line. Otherwise one couldn't click in that line at all. Need to be careful since ranges can become empty during editing. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
You are correct, I was in "e10s" mode and there was another process "plugin-container". Now I switched that off. I had noticed that my debugging had become strange, but couldn't figure out why. So thank you! Before considering changing the selection behaviour, I'd like to repeat my question, but I don't want to infuriate you ;-) The problem in this bug and in the other bug 1140617 is, that a line is continued with a new element. In this bug, someone clicks on a "DIV", since they click to the right of the text. The new text is inserted into a new element and loses the font. In the other bug, an image is inserted. After that the new text is inserted into a new element and loses the font. In both cases we could look in the inline frame and see whether there is a text element immediately before, whose attributes should be used. Is this an option? If we did that, we would *not* have to change the selection behaviour and wear all the possible consequences. Changing the selection will fix this bug but not the other bug 1140617. If this is not a viable solution, I can look further into changing the selection behaviour. My debugging works now: Debug when clicking on the text: >>> Entering nsFrame::HandlePress === in nsIFrame::GetContentOffsetsFromPoint === in GetSelectionClosestFrame === in nsFrame::GetSelectionClosestFrameForBlock >>> Entering nsFrameSelection::HandleClick === before BidiLevelFromClick >>> Endering nsTextFrame::GetChildFrameContainingOffset === nsTextFrame::GetChildFrameContainingOffset, offset=10 <<< Leaving nsTextFrame::GetChildFrameContainingOffset === after BidiLevelFromClick *** TypeInState::NotifySelectionChanged, container=05EF8900, offset=10 <-- the text of length 10 <<< Leaving nsFrameSelection::HandleClick after TakeFocus <<< Leaving nsFrame::HandlePress When clicking to the right of the text: >>> Entering nsFrame::HandlePress === in nsIFrame::GetContentOffsetsFromPoint === in GetSelectionClosestFrame === in nsFrame::GetSelectionClosestFrameForBlock === in nsFrame::GetSelectionClosestFrameForLine === in GetSelectionClosestFrame === in nsFrame::GetSelectionClosestFrameForBlock >>> Entering nsFrameSelection::HandleClick === before BidiLevelFromClick === after BidiLevelFromClick *** TypeInState::NotifySelectionChanged, container=066CF4C8, offset=2 <-- the DIV <<< Leaving nsFrameSelection::HandleClick after TakeFocus <<< Leaving nsFrame::HandlePress -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
I don't think it's reasonable to start traversing the DOM tree every time that we want to perform an editing operation to find the right styles/properties to use. It's a lot of unnecessary work. It should be a lot easier to normalize the selection in a sane way to prevent it from going into places where it would cause bugs like this, such as on a BR node when you click at the end of the line. If this approach proves to be impossible because of Web compatibility we can always re-evaluate. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
(In reply to Jorg K from comment #60) > The more conservative approach would be not to change the selection > behaviour but to maintain/re-establish the correct "type in state" after the > click. That is what IE does: The DIV is selected, but typing continues in > the "correct" font, see comment #54. Would you prefer this approach? No. TypeInState can only remember the properties set while the selection has not changed, so it is never the right fix for bugs that occur when you move the selection around in the document. > One could argue that clicking 10cm or 4" away from the text to the right > should not select the text. On the other hand, if no follows, then you > can click far to the right and the text is still selected. Also for text in > you can click far to the right and still select the text. Note that > links are not followed, even though the text is selected. That's the last > example. Sorry, I'm not really sure what you mean here. > Personally I would fire any web designer who creates websites for one > browser alone and who creates special code for somewhat far-fetched cases of > clicking a text. Or here. :-) (In reply to Jorg K from comment #61) > It's not going past static FrameTarget GetSelectionClosestFrameForLine () > http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#3540 > ... nor static FrameTarget GetSelectionClosestFrameForBlock nor static > FrameTarget GetSelectionClosestFrame. > > Also, very little processing in nsFrame::HandlePress. As I said before, > leaving there at line 2819: > http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#2819 Hmm, I wasn't guessing in comment 55, I actually ran this under the debugger, so I'm pretty sure you're doing something wrong. Not sure what exactly... > These functions fire when you hover over or click into the URL field (or the > search box), but NOT the rendered HTML. Hmm, maybe that's because you're attached to the wrong process? Does the active tab have an underline on its title? If yes, you're in e10s mode where we run the rendered content in a separate process. If this is what's happening, try either attaching the debugger to the content process, or open a new non-e10s window and load the test case there. > I'm not afraid of a large C++ code base, but I need a place to start. As I > said, five minutes of your time can possibly save me five hours or five days > of "reverse engineering". I'm trying to help, comment 55 should give you everything you need to get started. (In reply to Jorg K from comment #62) > As I said before, GetSelectionClosestFrameForLine is not called. The > following statements I placed into the corresponding functions in > nsFrame.cpp are not reached: > printf ("=== in GetSelectionClosestFrameForLine\n"); > printf ("=== in GetSelectionClosestFrameForBlock\n"); > printf ("=== in GetSelectionClosestFrame\n"); > printf ("=== in nsIFrame::GetContentOffsetsFromPoint\n"); This cannot possibly be true, I guarantee that. It's either you being attached to the wrong process or something else... > Further suggestions appreciated. I'd really like to find the code that > identifies the element under the cursor. Again, nsIFrame::GetContentOffsetsFromPoint(). > Also, is there a way to dump > out/print these data structures? How do you "print" an nsIFrame or an > nsBlockFrame? Yes! You can call nsIFrame::DumpFrameTree() on any frame in the frame tree to get a full dump of the frame tree (you can look up the specific frame you have by looking for its address in the frametree dump.) If you're under gdb or lldb, there is a helper command called ft, which dumps the frametree, otherwise you can just call that function under the debugger as |myFrame->DumpFrameTree()|. Also if you have a debug build, you can use the Layout Debugger under the Tools menu which gives you a GUI which dumps things such as the frame tree, the content (DOM) tree, etc. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Any idea why TypeInState::NotifySelectionChanged is never called when clicking around in a ? http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/TypeInState.cpp#75 Is that not a listener to listen to the selection changing? Maybe that has something to do with our problem(s) (thinking also of bug 1140617). -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Created attachment 8576320 A more comprehensive text The more conservative approach would be not to change the selection behaviour but to maintain/re-establish the correct "type in state" after the click. That is what IE does: The DIV is selected, but typing continues in the "correct" font, see comment #54. Would you prefer this approach? One could argue that clicking 10cm or 4" away from the text to the right should not select the text. On the other hand, if no follows, then you can click far to the right and the text is still selected. Also for text in you can click far to the right and still select the text. Note that links are not followed, even though the text is selected. That's the last example. Personally I would fire any web designer who creates websites for one browser alone and who creates special code for somewhat far-fetched cases of clicking a text. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
It's not going past static FrameTarget GetSelectionClosestFrameForLine () http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#3540 ... nor static FrameTarget GetSelectionClosestFrameForBlock nor static FrameTarget GetSelectionClosestFrame. Also, very little processing in nsFrame::HandlePress. As I said before, leaving there at line 2819: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#2819 These functions fire when you hover over or click into the URL field (or the search box), but NOT the rendered HTML. I'm not afraid of a large C++ code base, but I need a place to start. As I said, five minutes of your time can possibly save me five hours or five days of "reverse engineering". -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
(In reply to Jorg K from comment #51) > Sadly I don't understand some of what you wrote. Let me see what I > understood. > > You're saying you want to check how other rendering engines behave. I ran > the test from comment #37 on Chrome and IE. Both continue text entry with > the font present on the first line, so their behaviour is what I would > expect. That's not what I asked. I want to know where they put the selection. Attachment 8575104 kind of does that but I was hoping to get the offsets as well, but with the results in comment 54, it's clear that the exiting engines don't all agree on a single behavior, which means that my original idea may turn out to be incompatible with the Web. :/ What versions of these browsers did you test? What about Safari? Note that whether or not other engines have this bug is not relevant. > Next you talk about "test cases" where you click and dump out the selection. > In this case, we do a single click, so the selection will have one collapsed > range. I don't know which property of the Selection or Range you want to > inspect, maybe selRange.[start|end]Container.nodeName. Yeah, startContainer and endContainer, plus startOffset and endOffset. > I've stripped down the so-called Midas demo and added some information about > the clicking when carrying out the text case from comment #37. Result: > > FF: BODY, continues in the wrong font. > Chrome: #text, continues with the correct font. > IE: #text if you used between the lines, or BODY if you used > , continues with correct font in both cases. > > Let me know what to do next. And please, clear instructions ;-) The first step is to read and understand how GetContentOffsetsFromPoint() works, specifically the call to GetSelectionClosestFrame(), which is the function that decides what frame is marked as selected ultimately when a given frame receives a click event. For example in attachment 8575104 if you click to the right of the first line, we get to GetSelectionClosestFrameForLine() selecting the BRFrame at the end of the first line. Now, if we want to collapse the selection to the text frame, it means that we need to skip the BRFrame. So you should modify that logic to ignore BRFrame's if there is anything else on the same line before them (i.e., if there is something that we haven't yet looked at through the line iterator.) Then you should verify that the change actually makes us collapse the selection at the end of the text frame (so for example startContainer and endContainer should both be the text node and startOffset/endOffset should both be 10 depending on the length of the text frame). Then you should post the change to the try server and see what existing tests fail, and debug them to figure out why. There will be two classes of test failures, one for tests that are relying on the specific behavior we're changing here, so you should adjust those tests accordingly. Another set of test failures will hopefully catch the edge cases that we have not thought very carefully about yet. We should probably discuss the test failures once you get to that stage. Beware that this will probably be a lengthy process and it requires a lot of debugging skills and also being comfortable finding your way across a large C++ code base. Good luck! -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Created attachment 8576184 Alternate test, take 2, dumping out more info Results: FF: start=DIV(2) - end=DIV(2) IE 10 and 11 (current): start=DIV(2) - end=DIV(2) Chrome 41 (current): start=#text(10) - end=#text(10) Opera 28 (current): start=#text(10) - end=#text(10) Safari 5.1.7 (old): start=#text(10) - end=#text(10) -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
(In reply to Jorg K from comment #57) > Please confirm that you are happy to change FF's behaviour to be like > Chrome, Opera and Safari. > > I read the last section of your comment (quote: "... Good luck!") as a > confirmation, but before you were rather careful saying (1): > > If they all agree on putting the selection where I described above (...) > > then we can look into changing our code > and (2) > > it's clear that the exiting engines don't all agree on a single behavior, > > which > > means that my original idea may turn out to be incompatible with the Web. > > :/ I cannot predict how this change will pan out in the wild before we try it. It may very well be that we try this approach and it turns out that it breaks existing Web pages in a way that would cause us to revert the fix. It is only by trying this out that we can know for sure. > Regarding the first statement: They don't all agree, FF and IE differ, but > if FF moved into the Chrome camp, only IE would be left. And who knows what > the new "Spartan" browser will do. Well, what I meant was that the browsers do not have a consistent behavior here, so it could be that some websites are relying on Gecko's exact behavior here somehow. But like I said it's impossible to predict. > Regarding the second statement: I thought the original idea from comment #22 > was to move the selection into the text: > > My suspicion is that place is outside of the element that has the > > respective style > > for the font set. We should probably look into normalizing the selection > > in > > those cases to be inside the said element > Also in comment #50 you said: > > As I said in comment 22, we should probably look into normalizing the > > selection, > > so that if the line ends in an inline frame containing a text frame, we > > should > > put the selection at the end of the text frame as opposed to at the end of > > the inline > > frame terminating the line. > > I'm not sure how that would be (quote) "incompatible with the Web". > > Once I have your confirmation I will go ahead. > > For me, this is the single most annoying bug in Thunderbird which after 10 > years should finally be resolved. This affects more than just Thunderbird, and concerns behavior that is visible to Web pages. Web content can rely on the behavior of the engine in a lot of ways, for example by expecting that Firefox puts the selection somewhere specific when you go to the end of line. This is one of the reasons why fixing this bug is very difficult. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
>From someone who is 1% knowledgeable on this stuff... since the problem is that the cursor is returned, after clicking outside on the line being returned to, to a place outside the last (hidden) tag...which I have always seen to be ...could you not simply detect if that tag existed and place the cursor just before it? "For me, this is the single most annoying bug in Thunderbird which after 10 years should finally be resolved." AMEN, and thank you so much for working on it. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
I've done a little debugging. On every mouse move event we get this: >>> Entering PresShell::HandleEvent, frame=06EA9488 PresShell::HandleEvent calling nsLayoutUtils::GetEventCoordinatesRelativeTo PresShell::HandleEvent calling FindFrameTargetedByInputEvent PresShell::HandleEvent assigning frame = target, frame=1DD7F010 === about to call HandlePositionedEvent, frame=1DD7F010 === in PresShell::HandlePositionedEvent, mCurrentEventContent/Frame=0D066380/1DD7F010 <<< Leaving PresShell::HandleEvent right after HandlePositionedEvent (BTW, none of the frame numbers change when you move the cursor around. Unless you reload the page, they are always the same numbers). Surely somewhere in there it works out what is under the cursor, since when the cursor is over a link, things happen (cursor changes, link target is displayed). Sadly, I haven't yet found that code. If the button is clicked and released, we additionally get calls to HandlePress and HandleRelease >>> Entering PresShell::HandleEvent, frame=06EA9488 PresShell::HandleEvent calling nsLayoutUtils::GetEventCoordinatesRelativeTo PresShell::HandleEvent calling FindFrameTargetedByInputEvent PresShell::HandleEvent assigning frame = target, frame=1DD7F010 === about to call HandlePositionedEvent, frame=1DD7F010 === in PresShell::HandlePositionedEvent, mCurrentEventContent/Frame=0D066380/1DD7F010 === in nsFrame::HandlePress <<< Leaving PresShell::HandleEvent right after HandlePositionedEvent >>> Entering PresShell::HandleEvent, frame=06EA9488 PresShell::HandleEvent calling nsLayoutUtils::GetEventCoordinatesRelativeTo PresShell::HandleEvent calling FindFrameTargetedByInputEvent PresShell::HandleEvent assigning frame = target, frame=1DD7F010 === about to call HandlePositionedEvent, frame=1DD7F010 === in PresShell::HandlePositionedEvent, mCurrentEventContent/Frame=0D066380/1DD7F010 === in nsFrame::HandleRelease <<< Leaving PresShell::HandleEvent right after HandlePositionedEvent Here is the stack from the HandlePress call: nsFrame::HandlePress() Line 2776C++ nsFrame::HandleEvent() Line 2562C++ nsPresShellEventCB::HandleEvent() Line 507C++ mozilla::EventTargetChainItem::HandleEventTargetChain() Line 346C++ mozilla::EventDispatcher::Dispatch() Line 634C++ PresShell::DispatchEventToDOM() Line 8168C++ PresShell::HandleEventInternal() Line 8064C++ PresShell::HandlePositionedEvent() Line 7896C++ PresShell::HandleEvent() Line 7689C++ As I said before, GetSelectionClosestFrameForLine is not called. The following statements I placed into the corresponding functions in nsFrame.cpp are not reached: printf ("=== in GetSelectionClosestFrameForLine\n"); printf ("=== in GetSelectionClosestFrameForBlock\n"); printf ("=== in GetSelectionClosestFrame\n"); printf ("=== in nsIFrame::GetContentOffsetsFromPoint\n"); So the idea of discarding the BRFrame in GetSelectionClosestFrameForLine is perhaps not viable. Further suggestions appreciated. I'd really like to find the code that identifies the element under the cursor. Also, is there a way to dump out/print these data structures? How do you "print" an nsIFrame or an nsBlockFrame? Also, please answer the first question in comment #60 which suggested to not change the selected frame but to tweak the "type in state" like IE does. IE selects the DIV, but typing happens in the font of the preceding text element in the line. That may be an alternate way to fix the "lost font" problem that doesn't change existing selection behaviour. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Please confirm that you are happy to change FF's behaviour to be like Chrome, Opera and Safari. I read the last section of your comment (quote: "... Good luck!") as a confirmation, but before you were rather careful saying (1): > If they all agree on putting the selection where I described above (...) > then we can look into changing our code and (2) > it's clear that the exiting engines don't all agree on a single behavior, > which > means that my original idea may turn out to be incompatible with the Web. :/ Regarding the first statement: They don't all agree, FF and IE differ, but if FF moved into the Chrome camp, only IE would be left. And who knows what the new "Spartan" browser will do. Regarding the second statement: I thought the original idea from comment #22 was to move the selection into the text: > My suspicion is that place is outside of the element that has the respective > style > for the font set. We should probably look into normalizing the selection in > those cases to be inside the said element Also in comment #50 you said: > As I said in comment 22, we should probably look into normalizing the > selection, > so that if the line ends in an inline frame containing a text frame, we should > put the selection at the end of the text frame as opposed to at the end of > the inline > frame terminating the line. I'm not sure how that would be (quote) "incompatible with the Web". Once I have your confirmation I will go ahead. For me, this is the single most annoying bug in Thunderbird which after 10 years should finally be resolved. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
More results: Safari (5.1.7, had it on some old machine): Same behaviour as Chrome. Opera (27.0, fresh install): Same behaviour as Chrome. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Created attachment 8575104 Alternate test, much simpler Here is a much simplified test "alternate.htm". Results: FF: DIV, wrong font IE: DIV, correct font Chrome and Opera: #text, correct font. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Sadly I don't understand some of what you wrote. Let me see what I understood. You're saying you want to check how other rendering engines behave. I ran the test from comment #37 on Chrome and IE. Both continue text entry with the font present on the first line, so their behaviour is what I would expect. Next you talk about "test cases" where you click and dump out the selection. In this case, we do a single click, so the selection will have one collapsed range. I don't know which property of the Selection or Range you want to inspect, maybe selRange.[start|end]Container.nodeName. I've stripped down the so-called Midas demo and added some information about the clicking when carrying out the text case from comment #37. Result: FF: BODY, continues in the wrong font. Chrome: #text, continues with the correct font. IE: #text if you used between the lines, or BODY if you used , continues with correct font in both cases. Let me know what to do next. And please, clear instructions ;-) -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Created attachment 8574843 Stripped down Midas demo, also alerts on clicks and dumps out info -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
(In reply to Jorg K from comment #38) > Coming back to the suggestion from comment #25 and looking in > nsFrame::HandlePress. > > I traced it down into ns[Text]Frame::GetChildFrameContainingOffset with a > call stack of: > > nsFrame::GetChildFrameContainingOffset *or* > nsTextFrame::GetChildFrameContainingOffset > nsFrameSelection::GetFrameForNodeOffset > nsFrameSelection::BidiLevelFromClick > nsFrameSelection::HandleClick > nsFrame::HandlePress > > It varies whether nsFrame::GetChildFrameContainingOffset or > nsTextFrame::GetChildFrameContainingOffset is being called depending on > whether you click on the text, but almost to the left of the text, or > completely to the left of the text. > > When the nsTextFrame::GetChildFrameContainingOffsetAny version is called, it > offset is calculated correctly and the font is right. When the > nsFrame::GetChildFrameContainingOffset is called, the font is wrong. In the > latter case the caret still appears behind the text. Yes, this is what I was explaining in comment 22. The frame on which this function is called depends on the frame receiving the click event; if you click on the text it's going to be a textframe, otherwise it's going to be the parent frame (in the normal case as in the test case you describe above) or anything else that is under the mouse cursor. > Try it with a wide letter, like "m"! > > So in case a "Frame" being hit instead of a "TextFrame", perhaps the program > should look whether there is a text right before the caret once placed. No, we shouldn't change the way that we hit-test to find out which frame was clicked. As I said in comment 22, we should probably look into normalizing the selection, so that if the line ends in an inline frame containing a text frame, we should put the selection at the end of the text frame as opposed to at the end of the inline frame terminating the line. But before we can do that, we need to investigate the behavior of other engines in this situation (as in, we should see where they put the selection.) You need to create a test case which lets you click somewhere and have it dump out the place of the selection (which you can get through window.getSelection()). The simplest way to create such a test case is to set it up to print the selection after 10 seconds or so. Then you should document the behavior of IE/Chrome/Opera/Safari on that test case. If they all agree on putting the selection where I described above (IIRC some of the engines did that the last time I tested this years ago) then we can look into changing our code. Beware that it will probably take a significant amount of work adjusting everywhere in our code where we depend on the existing behavior, same in our existing tests. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
maybe-the-one - re comments 47 & 48: thanks for the clarifying explanation. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
>When I'm typing and correct something, the font size stays the same at the correction, but when I go to the end of the line to continue my typing, the font size is smaller, not larger." That's because there is a hidden tag at the end of the line (but before the spot where you are typing). When you move away from the line to do the correction, then click back to the right of the line in white space, the cursor has now been placed OUTSIDE the end font tag, so you have thus "lost" what font had previously been specified. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Oops, after a refresh of my source, behaviour of nsFrame::HandlePress has changed, now returns early: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#2816. Behaviour hasn't changed though. Click "far" left, font changes, click left/on, font is correct. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
[Bug 584632]
Jorg K - re Comment 36 (Just noticed: Not only the font gets lost, also the font size can get lost. For example when replying to a message in a small font, the size gets bigger when clicking at the end after correcting something in the same line.) When I'm typing and correct something, the font size stays the same at the correction, but when I go to the end of the line to continue my typing, the font size is smaller, not larger. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/584632 Title: composer changes font mid email To manage notifications about this bug go to: https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs