subject:"\[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]

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

2015-03-27 Thread Charles

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

2015-03-27 Thread Charles

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

sigh 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]

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]

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 @@
 +body
 +a target=_blank 
 href=https://bugzilla.mozilla.org/show_bug.cgi?id=756984;Mozilla Bug 
 756984/a
 +p id=display/p
 +div id=content style=display: none
 +/div

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 br 
 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 br 
 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]

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

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

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]

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]

2015-03-27 Thread Charles

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

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]

2015-03-27 Thread Malix

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]

2015-03-26 Thread Mozilla-jorgk

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

(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 span123/span 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

div id='div1'123br456br/div

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]

2015-03-26 Thread Mozilla-jorgk

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]

2015-03-26 Thread Mozilla-jorgk

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 span123/span 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]

(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: divblock 1brspannew line
 here/spandivblock2/div/div

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]

2015-03-25 Thread Aryeh Gregor

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

(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, currentFirstFrame, 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]

2015-03-25 Thread Aryeh Gregor

(In reply to Jorg K from comment #106)
 Pushed to try server (thanks guys for the support!):
 https://treeherder.mozilla.org/#/jobs?repo=tryrevision=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]

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]

2015-03-25 Thread Mkmelin+mozilla

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]

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]

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]

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]

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]

2015-03-25 Thread Aryeh Gregor

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]

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]

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=tryrevision=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]

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]

(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 brK - 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]

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 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, currentFirstFrame, 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=tryrevision=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]

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]

2015-03-24 Thread Aryeh Gregor

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]

2015-03-24 Thread Aryeh Gregor

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=tryrevision=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]

2015-03-24 Thread Mozilla-jorgk

Thanks for the suggestions made in comment #78. I'm testing with

spanbifont face=Courier newfoo bar on the same 
line/font/i/b/spanbr
spanbifont face=Arialfoo bar on the same 
line/font/i/b/spanbr
strongfoo bar on the same line/strongbr
This is sstrikethrough/sbr
25 msup2/supbr
Hsub2/subSOsub4/subbr
and some uunderline/ubr
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]

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]

(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 bfoo/b{}ibar/i, with the
 selection collapsed in between the b and i?  IIRC, WebKit in my testing
 forced this to be bfoo[]/bibar/i (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]

2015-03-24 Thread Mozilla-jorgk

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]

2015-03-24 Thread Mozilla-jorgk

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]

2015-03-24 Thread Aryeh Gregor

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]

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: divblock 1brspannew line
here/spandivblock2/div/div  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]

2015-03-24 Thread Charles

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]

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, currentBlockFrame);
 +  nsAutoLineIterator it = currentBlockFrame-GetLineIterator();
 +  it-GetLine(currentLine, currentFirstFrame, 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]

2015-03-23 Thread Ehsan-mozilla

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:

spanbifontfoo bar on the same line/font/i/b/span

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]

2015-03-23 Thread Aryeh Gregor

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 bfoo/b{}ibar/i, with the
selection collapsed in between the b and i?  IIRC, WebKit in my
testing forced this to be bfoo[]/bibar/i (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]

2015-03-22 Thread Aryeh Gregor

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]

2015-03-22 Thread Aryeh Gregor

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]

2015-03-22 Thread Mozilla-jorgk

@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]

2015-03-17 Thread Mozilla-jorgk

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]

2015-03-14 Thread Mozilla-jorgk

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]

2015-03-14 Thread Ehsan-mozilla

(In reply to Archaeopteryx [:aryx] from comment #70)
 Pushed to Try:
 https://treeherder.mozilla.org/#/jobs?repo=tryrevision=bb676357b6c9

Canceled this as it misses the most important part of the tests,
mochitests.  Repushed:

https://treeherder.mozilla.org/#/jobs?repo=tryrevision=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]

2015-03-14 Thread Ehsan-mozilla

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]

2015-03-14 Thread Archaeopteryx

Pushed to Try:
https://treeherder.mozilla.org/#/jobs?repo=tryrevision=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]

2015-03-13 Thread Ehsan-mozilla

(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 br follows, then you
 can click far to the right and the text is still selected. Also for text in
 p/p 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]

2015-03-13 Thread Mozilla-jorgk

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]

2015-03-13 Thread Ehsan-mozilla

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]

2015-03-13 Thread Mozilla-jorgk

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]

2015-03-13 Thread Mozilla-jorgk

Any idea why TypeInState::NotifySelectionChanged is never called when clicking 
around in a div contenteditable?
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]

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(args) Line 2776C++
nsFrame::HandleEvent(args) Line 2562C++
nsPresShellEventCB::HandleEvent(args) Line 507C++
mozilla::EventTargetChainItem::HandleEventTargetChain(args) Line 346C++
mozilla::EventDispatcher::Dispatch(args) Line 634C++
PresShell::DispatchEventToDOM(args) Line 8168C++
PresShell::HandleEventInternal(args) Line 8064C++
PresShell::HandlePositionedEvent(args) Line 7896C++
PresShell::HandleEvent(args) 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]

2015-03-12 Thread Fm-mozbugs

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 / font...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]

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]

2015-03-12 Thread Ehsan-mozilla

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

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]

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]

2015-03-12 Thread Ehsan-mozilla

(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 enter between the lines, or BODY if you used
 shiftenter, 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 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 br follows, then
you can click far to the right and the text is still selected. Also for
text in p/p 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]

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]

2015-03-10 Thread Ehsan-mozilla

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

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 enter between the lines, or BODY if you used 
shiftenter, 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 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]