[Bug 1847247]

[Ehsan-mozilla]

Hi Olivier,

Thanks for the bug report.  It has been a while since that patch was
landed so my memory may be a bit rusty, but I don't believe we
considered the use case of Linux distributions side-loading dictionaries
in this way...

Would it be a useful work-around to convert these dictionaries to the
[WebExtension dictionary format](https://developer.mozilla.org/en-
US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/dictionaries)?  I
haven't tested this myself, but at least in Firefox it is [possible to
side-load
extensions](https://extensionworkshop.com/documentation/publish
/distribute-sideloading/#standard-extension-folders), so perhaps
installing such extensions to a location like
`/usr/lib/thunderbird/extensions/{uuid}` may be a useful workaround?

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/1847247

Title:
  External dictionaries are not loaded

To manage notifications about this bug go to:
https://bugs.launchpad.net/thunderbird/+bug/1847247/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 1847772]

[Ehsan-mozilla]

(In reply to Jorg K (GMT+2) from comment #61)
> Hey Ehsan, thanks for your further comment. Let's face it: SeaMonkey is dead 
> beyond the equivalent of mozilla60 (sixty, no typo), which they haven't even 
> released yet. TB with about 10 staff has replaced all overlays, XBL bindings 
> and are now working on the XUL to XHTML transition. SM has done none of that 
> work, so at trunk, they can compile but not start :-(

Oh, sorry to hear about that.  :-/  I wasn't aware...

> - We could also have conditional compile for TB only. But since TB also 
> serves as a browser when used in HTML mode for RSS articles, I agree with 
> your argument.
> 
> Anyway, your further argument that other parts of Gecko could instantiate an 
> imap: URL and hence trigger the train of disaster that we're seeing now, and 
> that we can't "whack every mole", has convinced me :-)

Good to hear  :-).  Sorry to have pushed back, I know that it sucks to
receive pushback from a reviewer when they don't really know what better
solution to offer and they're effectively telling you "please spend more
time to come up with something else!", so I really appreciate you
considering my argument here.

> Yes, the waiting bit is going to be hard, since you're saying that I
should wait somewhere in IMAP land for some strings to arrive while
being called synchronously from the permission manager while creating an
imap: URI. I can't see how that would work.

Waiting for asynchronous stuff when something is called synchronously is
indeed hard, but it can sometimes be achieved with some care if you know
something about what it is that you're waiting for and what triggers it
to happen.  The way to do that is usually through setting up a nested
event loop to process incoming events to your thread (in this case the
main thread) until the condition you're waiting for (e.g. the strings
becoming available) has been met.

We have the `mozilla::SpinEventLoopUntil()` helper function for dealing
with these nested event loops, with many examples around the code base.
It looks like you're going on a different path now (which I admit I
don't fully understand!) but I thought I'd mention this in case it
proves useful anyway.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/1847772

Title:
  E-mail folder names are not localized in thunderbird 68

To manage notifications about this bug go to:
https://bugs.launchpad.net/thunderbird/+bug/1847772/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 1847772]

[Ehsan-mozilla]

(In reply to Jorg K (GMT+2) from comment #59)
> Thanks for your comments, Ehsan.
> 
> If you want to deliberate about what the real/actual problem is, here's my 
> view:
> 
> The root issue is that storageAccessAPI permissions are stored for non-web 
> origins, that is MailNews schemes like imap: or mailbox:. Do I read comment 
> #48 correctly and we agree on that? Quote: *"no it doesn't make sense for 
> principals representing imap/mailbox/or any other non-web URIs to have an 
> origin"*.

That is _one manifestation_ of the problem, but not the root issue.  The
root issue is that the permission manager allows storing permissions for
any URI scheme for any permission type.  The `storageAccessAPI`
permission type just happens to be the type of permission that trickled
the bug in this case, but if some other code today or tomorrow saves
other permission types for some other unexpected URI scheme in the
permission manager database you'd more or less have the exact same
effect as a result.

> I in fact know that TB needs image permission for http(s) and chrome
schemes and nothing else. I don't know what other permissions are needed
under the hood for the underlying Mozilla platform code, so in that
sense the patch could have been quite fatal ;-)

Your patch modifies SeaMonkey behaviour as well...

If you're going to rely on this knowledge, you should probably formalize
the expectations in the form of some `MOZ_RELEASE_ASSERT`s that crash
Thunderbird and SeaMonkey if any code ever violate your expectations
accidentally, and write some code to migrate the permission manager
database for existing installations since those may include the types of
permissions which newer builds may crash upon loading.  Given that
SeaMonkey should probably have a superset of Firefox's behaviour, and no
such expectations exist for Firefox, I have a very hard time that this
is a productive path to coming up with a good solution for this problem,
but you're welcome to pursue it.

> So would you accept a patch for TB (with #ifdef) to suppress
storageAccessAPI types for non-http(s): and chrome: schemes when reading
the permissions back from the database and potentially also a part that
would prevent those permissions from being stored in the first place?
That's my preferred solution.

No, that's a wallpaper solution that doesn't address the underlying
problem.  As I explained above, any patch that tries to solve this at
the level of the permission manager needs to crash Thunderbird and
SeaMonkey for invalid input into permission manager and upgrade the
database.  Again, this is *not* a good direction to solve the problem.

> Continuing the deliberation on the real problem:
> 
> The next issue is that the permissions manager instantiates nsIURI objects 
> when restoring the permissions from the database.
> 
> The next issue is that creating a new imap: URI runs a lot of MailNews code,
> https://searchfox.org/comm-central/rev/364e5ba7492c8a13e104662a7e43769819e339f7/mailnews/imap/src/nsImapService.cpp#2375
> including handling/initialising folders and hence trying to get localised 
> strings and store them in static memory **before** those strings are actually 
> available.
> https://searchfox.org/comm-central/rev/29706036071c4629c2b44512d1acecb64008cc46/mailnews/base/util/nsMsgDBFolder.cpp#2727
> 
> This part could be rephrased to saying: The problem is that the permissions 
> system is initialised before the language pack loads.

I understand all of this, and I think you need to think a bit outside of
the box here.  Think about what would have happened if it wasn't the
permission manager, but (for example) PSM, or DOM storage, or
safebrowsing, etc. etc. etc. which were trying to process a URI at
startup and somehow ended up having an imap URI on their hand.  It's not
even possible to find all such dependencies, let alone fix every single
similar bug by adding custom workarounds like the one you're suggesting
here.  What I'm trying to suggest to you instead is to figure out the
actual requirements of the IMAP service, whatever they may be, and write
the proper code to ensure that all dependencies are loaded _before_ we
need to process our first IMAP URI.  That way you wouldn't need to touch
the permission manager at all, and neither would you need to worry about
other similar cases where this problem may be showing up.

In other words...

> I don't know how you envisage to achieve (quote) *"making the code that is 
> reading this string wait on the loading of the thing that it is depending 
> on"*. Permissions code, MailNews folder initialisation code and loading the 
> strings from the language pack are completely independent. All I can imagine 
> is to listen to some "mail-startup-done" event and re-reading the 
> strings/folder names then.
> 
> So where from here?

... make the code reading this string (the IMAP service) wait on the
thing it is depending on (the localized strings) to become available[1].
:-)

[1] Maybe my understandi

[Bug 541951]

[Ehsan-mozilla]

(In reply to Mike Hommey [:glandium] (VAC: 31 Dec - 11 Jan) from comment #32)
> The syntax is a triviality of the patch. The core problem here is that
> there's no one to review the patch so that it possibly lands.

Can I help review?

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/541951

Title:
  Firefox 3.6 does not honour lockPref

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/541951/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 623844]

[Ehsan-mozilla]

(In reply to Mike Hommey [:glandium] (VAC: 31 Dec - 11 Jan) from comment #32)
> The syntax is a triviality of the patch. The core problem here is that
> there's no one to review the patch so that it possibly lands.

Can I help review?

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/623844

Title:
  lockpref not honored in /etc/thunderbird/pref/thunderbird.js

To manage notifications about this bug go to:
https://bugs.launchpad.net/thunderbird/+bug/623844/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 584632]

[Ehsan-mozilla]

(In reply to maybe-the-one from comment #138)
> Is there any way we can lobby for landing it in 38?

Please move that discussion to bug 1142879 or elsewhere.  Thanks!

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/584632

Title:
  composer changes font mid email

To manage notifications about this bug go to:
https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 584632]

[Ehsan-mozilla]

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

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/584632

Title:
  composer changes font mid email

To manage notifications about this bug go to:
https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 584632]

[Ehsan-mozilla]

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

[Ehsan-mozilla]

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]

[Ehsan-mozilla]

Comment on attachment 8583354
Test case (revised)

Review of attachment 8583354:
-

Looks like a great start!  Did you intend to test the rest of the cases
in a separate patch?

::: layout/generic/test/test_bug756984.html
@@ +13,5 @@
> +
> + href="https://bugzilla.mozilla.org/show_bug.cgi?id=756984";>Mozilla Bug 
> 756984
> +
> +
> +

Since you mentioned you're new to writing tests, I'll add some bits
about how this stuff works here.  I hope that helps!

The above is basically boilerplate that we include in most mochitests.
The only parts that are really required are the two script elements that
load the SimpleTest and event synthesis APIs.

@@ +25,5 @@
> +
> +/** Test for Bug 756984 **/
> +/** We test that clicking beyond the end of a line terminated with  
> selects the preceding text, if any **/
> +
> +SimpleTest.waitForExplicitFinish();

This is used to tell the test harness to not stop the test once the page
load finishes (which is the default.)  This is required for all tests
that can run past that point.  For example, this test is asynchronous
because of the waitForFocus call below.

@@ +27,5 @@
> +/** We test that clicking beyond the end of a line terminated with  
> selects the preceding text, if any **/
> +
> +SimpleTest.waitForExplicitFinish();
> +
> +SimpleTest.waitForFocus(function() {

This is required to ensure that the test window is raised to the top on
the desktop, which is needed to ensure proper event delivery.

@@ +35,5 @@
> +  var sel = window.getSelection();
> +  var f = document.getElementById("div1");
> +  theDiv.focus();
> +  sel.collapse(theDiv, 0);
> +  synthesizeMouse(theDiv, 100, 2, {type: "mousedown"}, f.contentWindow);

The last argument of synthesizeMouse is the window object corresponding
to the element that should receive the event.  If omitted, the default
is the current window object.  Now, f is an HTMLDivElement, which
doesn't have a contentWindow property, so f.contentWindow ends up as
undefined.  The reason this works is that from the perspective of the
callee function, omitted arguments are undefined, so that's what this
function checks for: .

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]

[Ehsan-mozilla]

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

[Ehsan-mozilla]

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]

[Ehsan-mozilla]

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

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

[Ehsan-mozilla]

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]

[Ehsan-mozilla]

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]

[Ehsan-mozilla]

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

[Ehsan-mozilla]

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]

[Ehsan-mozilla]

(In reply to Jorg K from comment #90)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #88)
> > Nit: you don't need to mention bug numbers in comments.  This information is
> > available through hg/git blame.
> There are many bug numbers in the code (including some that you entered for
> bug 596506), so this policy must have changed recently.

Well you're repeating it multiple times in the same file.  The bug
number is really not adding any useful information in this case.

> > Also, please wrap if bodies in braces.
> Again, the policy seems to have changed here since there are many conditions
> of this style:
> if (!range.content)
>   return NS_ERROR_FAILURE;

We unfortunately don't consistently adhere to the coding style, but we
should for new code that we're adding.

> > There is already code which gets the line frame count earlier in this
> > function.  Please refactor it out of the else block it currently lives in
> > and just use lineFrameCount from that code here.  That already takes care of
> > checking the return value of GetLine for errors too.  Also, I think you want
> > to check atLineEdge instead of aJumpLines here.
> I disagree. I did what you suggest, but it didn't work. The
> it->GetLine(thisLine, &firstFrame, &lineFrameCount, nonUsedRect);
> ealier on gets the details for the previous line where the left arrow was
> pressed.
> We then move on to the line before. The next
> it->GetLine(currentLine, ¤tFirstFrame, &lineFrameCount, usedRect);
> is operating on the line we've just moved to, the one where we want to skip
> the brFrame.
> The only thing which would be a bit cleaner is not to re-use the "it"
> variable but to declare another one.

Oh yeah, good point.  So please make sure you check the return value of
GetLine like above.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/584632

Title:
  composer changes font mid email

To manage notifications about this bug go to:
https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 584632]

[Ehsan-mozilla]

Comment on attachment 8582360
Patch to fix all three problems: Click, "end" key, left arrow from start of 
previous line

Review of attachment 8582360:
-

Some feedback on your patch so far.

::: layout/generic/nsFrame.cpp
@@ +3555,3 @@
>for (int32_t n = aLine->GetChildCount(); n;
> --n, frame = frame->GetNextSibling()) {
> +// Bug 756984: Skip brFrames. Can only skip if the line contains at 
> least one selectable and non-empty frame before

Nit: you don't need to mention bug numbers in comments.  This
information is available through hg/git blame.

@@ +3555,4 @@
>for (int32_t n = aLine->GetChildCount(); n;
> --n, frame = frame->GetNextSibling()) {
> +// Bug 756984: Skip brFrames. Can only skip if the line contains at 
> least one selectable and non-empty frame before
> +if (!SelfIsSelectable(frame, aFlags) || frame->IsEmpty() || (canSkipBr 
> && frame->GetType() == nsGkAtoms::brFrame))

Nit: please wrap lines at 80 characters.  For example, according to our
coding style, this condition should be written as:

  if (!SelfIsSelectable(frame, aFlags) || frame->IsEmpty() ||
  (canSkipBr && frame->GetType() == nsGkAtoms::brFrame))

@@ +6801,5 @@
>  for (int32_t count = lineFrameCount; count;
>   --count, frame = frame->GetNextSibling()) {
>if (!frame->IsGeneratedContentFrame()) {
> +// Bug 756984: When jumping to the end of the line with the 
> "end" key, skip over brFrames
> +if (endOfLine && lineFrameCount > 1 && frame->GetType() == 
> nsGkAtoms::brFrame) continue;

Nit: again, please adjust the whitespace here.  Also, please wrap if
bodies in braces.

@@ +7090,5 @@
> +  nsIFrame *currentBlockFrame, *currentFirstFrame;
> +  nsRect usedRect;
> +  int32_t currentLine = nsFrame::GetLineNumber(traversedFrame, 
> aScrollViewStop, ¤tBlockFrame);
> +  nsAutoLineIterator it = currentBlockFrame->GetLineIterator();
> +  it->GetLine(currentLine, ¤tFirstFrame, &lineFrameCount, 
> usedRect);

There is already code which gets the line frame count earlier in this
function.  Please refactor it out of the else block it currently lives
in and just use lineFrameCount from that code here.  That already takes
care of checking the return value of GetLine for errors too.  Also, I
think you want to check atLineEdge instead of aJumpLines here.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/584632

Title:
  composer changes font mid email

To manage notifications about this bug go to:
https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 584632]

[Ehsan-mozilla]

Looks like Aryeh answered your questions (thanks Aryeh!)

I realized that I forgot about another case that we need to test.  br
frames are not the only reason for a line ending, we can also get line
breaks at block boundaries, for example: block 1new line
hereblock2  We need to ensure that the selection
ends up on the text node when you put the selection at the end of "new
line here" using any of the methods mentioned before.  Again, please
note that some of those cases may already work properly and/or be fixed
by your patch, but it's worth testing.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/584632

Title:
  composer changes font mid email

To manage notifications about this bug go to:
https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 584632]

[Ehsan-mozilla]

(In reply to :Aryeh Gregor from comment #77)
> For the record, from black-box testing of WebKit a few years ago, it looked
> like it normalized the selection after every change.  Even if you called
> .addRange(), it copied the range and then stuck the selection endpoints
> inside a nearby text node if available, etc.  I think it's taking things too
> far to change script-specified selections

Yeah, I think that's a bit too aggressive, and probably would cause
authors to have to do insane hacks if they actually want to put the
selection where the engine doesn't think is appropriate.

> but the right way to do this is
> probably to have some sort of helper method in Selection like
> NormalizePoint(nsINode*, int32_t) and call it before every user-initiated
> selection change.  We might want to disallow other types of user-created
> selections from occurring in the future, although my brain is too rusty to
> supply any.

Well, most of the code handling selection changes actually lives outside
Selection.  ;-)  Depending on where we need to modify the behavior,
having this kind of helper may or may not help...  See Jorg's patch for
example, I think the way he modifies the existing logic is cleaner than
going with the current logic and then fixing it up elsewhere.

> Do we want to allow a selection like foo{}bar, with the
> selection collapsed in between the  and ?  IIRC, WebKit in my testing
> forced this to be foo[]bar (always making it on the previous
> text node).  A ten-second test in WordPad suggests this is the right thing
> to do.

Yes, that makes sense to me.  Follow-up bug perhaps?

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/584632

Title:
  composer changes font mid email

To manage notifications about this bug go to:
https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 584632]

[Ehsan-mozilla]

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]

[Ehsan-mozilla]

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]

[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  which then gets us
through the same machinery as you've seen before.

Another thing that would be useful to test (but I'm pretty sure that it
would work correctly based on the code changes) is what happens in both
of these cases when you have several inline elements nested underneath
each other, such as:

foo bar on the same line

You should ensure that the selection is again put at the end of the text
frame in this case, so that when the user starts to type again, the new
text would have the correct styles imposed by all of those inline
elements.

Once you've verified and fixed all of these cases, the next step would
be to write a unit test as Aryeh mentioned that examines this behavior
and ensures that we don't end up regressing it in the future.  In
addition to simulating a mouse click, you'd need to use synthesizeKey,
for example, synthesizeKey("VK_UP") for simulating the "Up" key, and so
on.

And thanks again for working on this!

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/584632

Title:
  composer changes font mid email

To manage notifications about this bug go to:
https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 584632]

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

[Ehsan-mozilla]

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

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

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

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/584632

Title:
  composer changes font mid email

To manage notifications about this bug go to:
https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 584632]

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

[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  follows, then you
> can click far to the right and the text is still selected. Also for text in
>  you can click far to the right and still select the text. Note that
> links are not followed, even though the text is selected. That's the last
> example.

Sorry, I'm not really sure what you mean here.

> Personally I would fire any web designer who creates websites for one
> browser alone and who creates special code for somewhat far-fetched cases of
> clicking a text.

Or here.  :-)

(In reply to Jorg K from comment #61)
> It's not going past static FrameTarget GetSelectionClosestFrameForLine ()
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#3540
> ... nor static FrameTarget GetSelectionClosestFrameForBlock nor static
> FrameTarget GetSelectionClosestFrame.
> 
> Also, very little processing in nsFrame::HandlePress. As I said before,
> leaving there at line 2819:
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#2819

Hmm, I wasn't guessing in comment 55, I actually ran this under the
debugger, so I'm pretty sure you're doing something wrong.  Not sure
what exactly...

> These functions fire when you hover over or click into the URL field (or the
> search box), but NOT the rendered HTML.

Hmm, maybe that's because you're attached to the wrong process?  Does
the active tab have an underline on its title?  If yes, you're in e10s
mode where we run the rendered content in a separate process.  If this
is what's happening, try either attaching the debugger to the content
process, or open a new non-e10s window and load the test case there.

> I'm not afraid of a large C++ code base, but I need a place to start. As I
> said, five minutes of your time can possibly save me five hours or five days
> of "reverse engineering".

I'm trying to help, comment 55 should give you everything you need to
get started.

(In reply to Jorg K from comment #62)
> As I said before, GetSelectionClosestFrameForLine is not called. The
> following statements I placed into the corresponding functions in
> nsFrame.cpp are not reached:
> printf ("=== in GetSelectionClosestFrameForLine\n");
> printf ("=== in GetSelectionClosestFrameForBlock\n");
> printf ("=== in GetSelectionClosestFrame\n");
> printf ("=== in nsIFrame::GetContentOffsetsFromPoint\n");

This cannot possibly be true, I guarantee that.  It's either you being
attached to the wrong process or something else...

> Further suggestions appreciated. I'd really like to find the code that
> identifies the element under the cursor.

Again, nsIFrame::GetContentOffsetsFromPoint().

> Also, is there a way to dump
> out/print these data structures? How do you "print" an nsIFrame or an
> nsBlockFrame?

Yes!  You can call nsIFrame::DumpFrameTree() on any frame in the frame
tree to get a full dump of the frame tree (you can look up the specific
frame you have by looking for its address in the frametree dump.)  If
you're under gdb or lldb, there is a helper command called ft, which
dumps the frametree, otherwise you can just call that function under the
debugger as |myFrame->DumpFrameTree()|.

Also if you have a debug build, you can use the Layout Debugger under
the Tools menu which gives you a GUI which dumps things such as the
frame tree, the content (DOM) tree, etc.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/584632

Title:
  composer changes font mid email

To manage notifications about this bug go to:
https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 584632]

[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  between the lines, or BODY if you used
> , continues with correct font in both cases.
> 
> Let me know what to do next. And please, clear instructions ;-)

The first step is to read and understand how
GetContentOffsetsFromPoint() works, specifically the call to
GetSelectionClosestFrame(), which is the function that decides what
frame is marked as selected ultimately when a given frame receives a
click event.

For example in attachment 8575104 if you click to the right of the first
line, we get to GetSelectionClosestFrameForLine() selecting the BRFrame
at the end of the first line.  Now, if we want to collapse the selection
to the text frame, it means that we need to skip the BRFrame.  So you
should modify that logic to ignore BRFrame's if there is anything else
on the same line before them (i.e., if there is something that we
haven't yet looked at through the line iterator.)  Then you should
verify that the change actually makes us collapse the selection at the
end of the text frame (so for example startContainer and endContainer
should both be the text node and startOffset/endOffset should both be 10
depending on the length of the text frame).  Then you should post the
change to the try server and see what existing tests fail, and debug
them to figure out why.  There will be two classes of test failures, one
for tests that are relying on the specific behavior we're changing here,
so you should adjust those tests accordingly.  Another set of test
failures will hopefully catch the edge cases that we have not thought
very carefully about yet.  We should probably discuss the test failures
once you get to that stage.

Beware that this will probably be a lengthy process and it requires a
lot of debugging skills and also being comfortable finding your way
across a large C++ code base.  Good luck!

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/584632

Title:
  composer changes font mid email

To manage notifications about this bug go to:
https://bugs.launchpad.net/thunderbird/+bug/584632/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 584632]

[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 1322784]

[Ehsan-mozilla]

I added this change to the patches directory:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99814e9730de

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/1322784

Title:
  Firefox crashes in flag_qsort during spellchecker initialization on
  x86 due to gcc bug

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/1322784/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 584632]

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

[Ehsan-mozilla]

(In reply to Jorg K from comment #26)
> Re. your last comment:
> > I _think_ to fix that part you need to get Thunderbird tell Gecko about how
> > to format the new paragraph. 
> 
> I tried with a  in Firefox. The editor handles
> insertion of images by itself. So the question is: How could Thunderbird,
> the invoker of the editor, be notified, so it could in turn do whatever it
> does normally (I need to investigate what that is exactly) to communicate
> that the format required after the insertion?

If it works with a simple contenteditable element in Firefox, then
Thunderbird is doing something that it should not be doing.  I have no
idea what that might be though.  But at any rate, that is off topic for
this bug.

> Can an callback be installed
> that the editor calls when new nodes are created in the DOM tree?

You can use a mutation observer to be notified when new nodes are
injected into the tree, but like I said, that doesn't help if
Thunderbird is doing something that it should not be doing.

> So far I get the following picture of the interaction between Thunderbird
> and the editor:
> 1) Thunderbird invokes the editor and communicates the initial conditions,
> font, size, etc.
> 2) The editor does all the editing. When new nodes are created in the DOM,
> no "special" style element is used, so to the Thunderbird user it looks like
> the style gets lost.
> 3) No callback to Thunderbird takes place while the editor is running.
> 4) Thunderbird may send "commands" to the editor, to change the font, bold,
> etc.
> 5) When the editing is done, Thunderbird receives a copy of the DOM (or the
> plain text or HTML) and stores it or sends it.

I'm not familiar enough with the Thunderbird side of things to be able
to tell how it interacts with Gecko.  And I don't know anyone who does.
Your best bet may be reading the code.

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

[Ehsan-mozilla]

(In reply to Jorg K from comment #28)
> Sorry to trouble you again. I have some more questions to understand how the
> architecture hangs together. Let me summarise the questions from the
> previous posts here:
> 
> Where is the mouse click translated into identifying a node of the DOM tree?
> Your answer was nsFrame::HandlePress. I looked through there in the debugger
> but didn't see anything connected to the editor.

Yes, that part has nothing to do with the editor.  In general, the
editor is usually oblivious to selection changes, caret movement and
such.

> nsFrame::HandlePress
> handles clicks anywhere on the mail composition window. I would like to find
> the spot where the editor looks through the DOM tree to identify the node. I
> looked in nsHTMLEditorEventListener::MouseDown, but in there only the very
> last branch controlled by "else if (!isContextClick && buttonNumber == 0 &&
> clickCount == 1)" is executed. I would imagine to find a traversal of the
> DOM tree to find the correct node.

I'm not exactly sure why you're looking at the editor code.  Nothing in
comment 22 will probably need to be fixed in the editor code.

> How does Thunderbird communicate "composition font" or "default font" to the
> editor?
> Your answer was that you didn't know. Fair enough. You suggested the
> document.execCommand could be used. Assuming that this is implemented in
> nsHTMLDocument::ExecCommand, I set a breakpoint which wasn't reached.
> Equally, then the UI controls for bold, italics, font, etc. are operated,
> the breakpoint wasn't reached. I'd like to know how this is passed to the
> editor. So I will ask in the Thunderbird group.

I'd be curious to know, FWIW!

> When the user writes an e-mail and clicks into a text, the UI controls are
> updated, that is the indicators for font, bold, italics, etc. reflect the
> properties of the text where the user clicked.
> How is this done? How does the editor notify the UI, after identifying the
> node clicked and the finding the properties of the node. This is also
> important to know, because in cases where the font gets lost, the UI is not
> updated correctly.

Again, I know nothing about what Thunderbird does, but a normal web
application which does this would probably use
document.queryCommandEnabled and friends.  I think you should be looking
at the Thunderbird code to see how it does things though, there is no
point in guessing!

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

[Ehsan-mozilla]

(In reply to Jorg K from comment #23)
> I could use some help to locate the code that translates the click into
> identifying the element. Somewhere in ns(HTML)EditorEventListener.cpp, I
> suppose.

nsFrame::HandlePress is called when you click on an element, and at
least part of the selection normalization should probably happen inside
there.

(In reply to Jorg K from comment #24)
> I am having second thoughts. If I understood correctly, the idea is to
> continue adding to the pre-existing element at the end of the line instead
> of adding a new element which will lose the style or font. Is this really
> the correct approach?

It's probably the easiest way to fix this bug.  There are probably other
implementation strategies as well, but I can't think of an easier one.

> I have three more questions in this context:
> 1) The paragraph is created when replying to an e-mail does have the correct
> style/font. It only gets lost after moving the caret around in the e-mail.
> How does Thunderbird communicate to the editor which style to use in the
> first place, in the case comment #23, the ?

I don't know how Thunderbird works, but this information is usually
transferred to Gecko using document.execCommand calls.

> Why can't this method be
> used for any subsequently created text elements? 

I don't understand this question.

Note that the type-in state code is mostly used to handle sequences such
as calling document.execCommand("bold") and starting to type.  It
doesn't know how to handle anything more complicated (for example
remembering the styles to be used for an arbitrary element at an
arbitrary DOM position.)

> Currently further text
> elements are created with the "default" font, which is also configured in
> Thunderbird and must be communicated somehow to the editor.

Again, I don't know the Thunderbird specific parts here.

> 2) Over in bug 1100966 (spell checker losing red underlines) another node is
> also created when clicking at the end of the line. Upon backspace, this node
> becomes empty and is merged (badly) with the preceding one, which in turn
> loses the underlines. Why didn't we consider to continue the existing node
> instead of creating a new one? In this case the merge would not happen.

Because it's possible to fix that code more easily, and also we use the
same joining code for other purposes such as when two nodes really need
to be joined because the content in between them is being deleted.
Fixing this bug would make the steps to reproduce in that bug change,
but these are separate issues.

> 3) Another problem with losing the style/font can be observed after
> inserting an image, see comment #14. I think the proposed approach of
> reusing a pre-existing text element can't be used here, since the preceding
> element is an image. So wouldn't it be easier to make sure the chosen
> style/font is observed when creating another text element after the image?

I _think_ to fix that part you need to get Thunderbird tell Gecko about
how to format the new paragraph.  The fix suggested in comment 22 will
not fix all of the issues with the type-in style being lost.

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

[Ehsan-mozilla]

(In reply to comment #28)
> We should disable the screen save for non-fullscreen playback too.

Why?  Some websites use  as an element in their design these days
(for example, as the page background.)  It seems counter intuitive for
such a website to disable the screen saver!

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/434476

Title:
  screensaver starts while playing HTML5 videos

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/434476/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 434476]

[Ehsan-mozilla]

(In reply to comment #31)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailacopolypse) from comment #29)
> > (In reply to comment #28)
> > > We should disable the screen save for non-fullscreen playback too.
> > 
> > Why?  Some websites use  as an element in their design these days
> > (for example, as the page background.)  It seems counter intuitive for such
> > a website to disable the screen saver!
> 
> Because sometimes you want to watch a video non-fullscreen with duration 
> longer
> than your screensaver's timeout? I certainly don't fullscreen all the videos I
> watch.
> 
> I think it would be hard to programmatically distinguish between the non
> fullscreen foreground case and all permutations of the 
> video-as-page-background
> use case you describe.

Can we at least do that for videos which have started playback as a
result of a user action?

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/434476

Title:
  screensaver starts while playing HTML5 videos

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/434476/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 1051559]

[Ehsan-mozilla]

Backed out because of mochitest-1 timeouts on Linux:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e1acd3b9ce8

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/1051559

Title:
  Build Firefox with GStreamer support

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/1051559/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 1155934]

[Ehsan-mozilla]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f5bf6a0d04da

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/1155934

Title:
  Firefox does NOT copy images when History is set to 'Never remember'

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/1155934/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 1155934]

[Ehsan-mozilla]

(In reply to stevend811 from comment #3)
> (In reply to Matthias Versen [:Matti] from comment #1)
> > The global private browsing mode got replaced by a per window private
> > browsing mode.
> > Could you test a nightly build from mozilla.org ?
> 
> I tested it on the nightly build and the bug is occurring there as well.

Yeah, this affects all versions of Firefox.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/1155934

Title:
  Firefox does NOT copy images when History is set to 'Never remember'

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/1155934/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 1155934]

[Ehsan-mozilla]

Created attachment 725915
Patch (v1)

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/1155934

Title:
  Firefox does NOT copy images when History is set to 'Never remember'

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/1155934/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 1155934]

[Ehsan-mozilla]

This is not a regression and as far as I can tell, this has never worked
properly on Linux.  The problem is that on Linux we have both the
selection and the global clipboards, so
nsClipboardPrivacyHandler::PrepareDataForClipboard gets called twice,
and the second call to nsITransferable::AddDataFlavor fails since the
NS_MOZ_DATA_FROM_PRIVATEBROWSING flavor already exists on the
transferable, and then we bail out of the function and do not attempt to
do anything further.

The reason that copying text works is that we don't attempt to set the
selection clipboard data when you copy text, so the second AddDataFlavor
call is never made.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/1155934

Title:
  Firefox does NOT copy images when History is set to 'Never remember'

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/1155934/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 434476]

[Ehsan-mozilla]

*** Bug 772347 has been marked as a duplicate of this bug. ***

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/434476

Title:
  screensaver starts while playing HTML5 videos

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/434476/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 788102]

[Ehsan-mozilla]

Firefox 12 and 13 have shipped, no point in tracking this bug for them.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/788102

Title:
  Firefox crashes or hangs on GMail

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/788102/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 107247]

[Ehsan-mozilla]

(In reply to garryb from comment #70)
> Woot, I see this now in Firefox 12. (This is the main reason we're still
> using a designMode iframe in G+)

Great!  Please file new bugs if you see more problems blocking you from
moving to contenteditable elements.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/107247

Title:
  Launchpad bug pages trigger caret browsing in Firefox and other Gecko
  browsers

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/107247/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 412647]

[Ehsan-mozilla]

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

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/412647

Title:
  Firefox is not able to play mp4  tags

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/412647/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 303269]

[Ehsan-mozilla]

If you have steps to reproduce, please file a new bug and we'll discuss
it there.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/303269

Title:
  Automatically select language for spell check based on user input

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/303269/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 918678]

[Ehsan-mozilla]

(In reply to amai from comment #26)
> BTW, if the code inside hunspell is responsible (it seems to be) shouldn't
> we report on their project also??

We could do that too, but without somebody being able to reproduce this,
there is a little chance that it will get fixed.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/918678

Title:
  Constant Firefox crashes with Thai language installed

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/918678/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 918678]

[Ehsan-mozilla]

Yes, I did install that package.  I don't see a dictionaries directory
under /usr/lib64/firefox/extensions/langpack...@firefox.mozilla.org/ at
all. :/

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/918678

Title:
  Constant Firefox crashes with Thai language installed

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/918678/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 107247]

[Ehsan-mozilla]

(In reply to Alex Keybl [:akeybl] from comment #63)
> Bug 696020 appears to have exacerbated this problem in 10 (see bug 702064).
> The attached patch appears to be higher risk than we're comfortable for Beta
> (please correct me if I'm wrong), but I'm wondering if this is a good
> candidate for Aurora 11.

I'd be much more comfortable if we could back out bug 696020 from
Aurora.  smaug, can we do that?

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/107247

Title:
  Launchpad bug pages trigger caret browsing in Firefox and other Gecko
  browsers

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/107247/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 107247]

[Ehsan-mozilla]

*** Bug 702064 has been marked as a duplicate of this bug. ***

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/107247

Title:
  Launchpad bug pages trigger caret browsing in Firefox and other Gecko
  browsers

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/107247/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 107247]

[Ehsan-mozilla]

Neil, I think you need to add those commands back, and just make us not
use them... :(

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/107247

Title:
  Launchpad bug pages trigger caret browsing in Firefox and other Gecko
  browsers

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/107247/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 107247]

[Ehsan-mozilla]

Comment on attachment 570250
Fixed patch

Review of attachment 570250:
-

I'm not crazy about "editor" vs. "editing" controllers, but I couldn't
really think of anything better, so r=me.

Thanks a lot for your work on this, Neil, and sorry that I did a very
bad job at responding to the review request in time.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/107247

Title:
  Launchpad bug pages trigger caret browsing in Firefox and other Gecko
  browsers

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/107247/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 107247]

[Ehsan-mozilla]

Comment on attachment 569554
Proposed patch

Review of attachment 569554:
-

Looks very good!

Does my test case pass with this patch?

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/107247

Title:
  Launchpad bug pages trigger caret browsing in Firefox and other Gecko
  browsers

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/107247/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 602877]

[Ehsan-mozilla]

https://hg.mozilla.org/mozilla-central/rev/0a6b707742dd

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/602877

Title:
  Position is not being updated when atk_text_set_caret_offset is used

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/602877/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 107247]

[Ehsan-mozilla]

Neil, does your patch make the test part of attachment 548924 pass?  If
so, we can get them landed.  :-)

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/107247

Title:
  Launchpad bug pages trigger caret browsing in Firefox and other Gecko
  browsers

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/107247/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 107247]

[Ehsan-mozilla]

Ping?

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/107247

Title:
  Launchpad bug pages trigger caret browsing in Firefox and other Gecko
  browsers

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/107247/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 294712]

[Ehsan-mozilla]

Try pinging him on IRC?  He may not be watching his bugmail closely...

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/294712

Title:
  Firefox does not display images when "Show image" is selected if auto
  display of images is disabled

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/294712/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 294712]

[Ehsan-mozilla]

Couldn't we just suppress checking the blocking status inside
nsImageLoadingContent::LoadImage?  This way we wouldn't need to modify
the content blocker service at all, we just wouldn't query it if we're
doing a force-reload.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/294712

Title:
  Firefox does not display images when "Show image" is selected if auto
  display of images is disabled

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/294712/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 838322]

[Ehsan-mozilla]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/463dbdc80866
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d19ac6a6ef00

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/838322

Title:
  Remove the exemptions for the Staat der Nederlanden root

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/838322/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 838322]

[Ehsan-mozilla]

http://hg.mozilla.org/releases/mozilla-release/rev/e65f4c8bd243
http://hg.mozilla.org/releases/mozilla-release/rev/5b6c2f8ff6da
http://hg.mozilla.org/releases/mozilla-release/rev/14452010e012

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/838322

Title:
  Remove the exemptions for the Staat der Nederlanden root

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/838322/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 838322]

[Ehsan-mozilla]

http://hg.mozilla.org/releases/mozilla-beta/rev/01d409d49c6a
http://hg.mozilla.org/releases/mozilla-beta/rev/ff20a21364bb

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/838322

Title:
  Remove the exemptions for the Staat der Nederlanden root

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/838322/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 838322]

[Ehsan-mozilla]

Also: http://hg.mozilla.org/releases/mozilla-aurora/rev/a5a5c583c381

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/838322

Title:
  Remove the exemptions for the Staat der Nederlanden root

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/838322/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 838322]

[Ehsan-mozilla]

Also: http://hg.mozilla.org/mozilla-central/rev/5319db188180

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/838322

Title:
  Remove the exemptions for the Staat der Nederlanden root

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/838322/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 838322]

[Ehsan-mozilla]

http://hg.mozilla.org/mozilla-central/rev/471f4fbc9c85
http://hg.mozilla.org/releases/mozilla-aurora/rev/f020f92c79ca
http://hg.mozilla.org/releases/mozilla-beta/rev/f6dafd2dcc63
http://hg.mozilla.org/releases/mozilla-beta/rev/731b7bc62da3
http://hg.mozilla.org/releases/mozilla-release/rev/c32149f14aeb
http://hg.mozilla.org/releases/mozilla-release/rev/58b06f58f4f8
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2e7eba4287e7
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/dbfc18ea5b93

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/838322

Title:
  Remove the exemptions for the Staat der Nederlanden root

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/838322/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 107247]

[Ehsan-mozilla]

(In reply to n...@parkwaycc.co.uk from comment #24)
> > Is the editor attaching it's command controller to the document and not the
> > editable area?
> Correct. There was some issue with attaching the commands to the editable
> area which ehsan has pointed me towards several times but I still forget the
> bug #.

I tried very hard in bug 289384 to do that, but I failed miserably...
Maybe because I didn't have a good idea what I'm doing. :/

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/107247

Title:
  Launchpad bug pages trigger caret browsing in Firefox and other Gecko
  browsers

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/107247/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 303269]

[Ehsan-mozilla]

Comment on attachment 553143
part2: do not override manually set dictionary

>+  PRBool mUpdateDictionaryRunning;
>+  PRBool mDictWasSetManually;

Please use PRPackedBool.

>+class UpdateDictionnaryHolder {
>+  private:
>+nsEditorSpellCheck* mSpellCheck;
>+  public:
>+UpdateDictionnaryHolder(nsEditorSpellCheck* esc): mSpellCheck(esc) {
>+  if (mSpellCheck) {
>+mSpellCheck->BeginUpdateDictionary();
>+  }
>+}
>+~UpdateDictionnaryHolder() {
>+  if (mSpellCheck) {
>+mSpellCheck->EndUpdateDictionary();
>+  }
>+}
> };

Also, please move this class to the cpp file.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/303269

Title:
  Automatically select language for spell check based on user input

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/303269/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 303269]

[Ehsan-mozilla]

Followup patches landed:

http://hg.mozilla.org/mozilla-central/rev/e4a7c48aff3a
http://hg.mozilla.org/mozilla-central/rev/180442fd6448

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/303269

Title:
  Automatically select language for spell check based on user input

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/303269/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 303269]

[Ehsan-mozilla]

Comment on attachment 553150
part3: fix Get/SetCurrentDictionary signature

This really belongs to another bug...  but r=me nevertheless!

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/303269

Title:
  Automatically select language for spell check based on user input

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/303269/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 303269]

[Ehsan-mozilla]

Landed on mozilla-inbound.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/303269

Title:
  Automatically select language for spell check based on user input

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/303269/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 303269]

[Ehsan-mozilla]

Comment on attachment 552655
patch v2.7

Review of attachment 552655:
-

This looks great!  Thanks a lot for your hard work 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/303269

Title:
  Automatically select language for spell check based on user input

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/303269/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 303269]

[Ehsan-mozilla]

Folks, *please* do not comment on this bug in order to discuss other
bugs.  This distracts the people who are working on this bug (like me
and Arno) and also makes the discussion impossible to find for somebody
who's looking at the other bug.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/303269

Title:
  Automatically select language for spell check based on user input

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/303269/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 303269]

[Ehsan-mozilla]

(In reply to comment #89)
> I run the test on my system before submitting a patch either here, or on try
> server. Tests can succeed on my machine but fail in other environments. For
> example, as I told in comment 78, this can be caused by different font
> settings.

Hmm, I was talking about this failure
http://tinderbox.mozilla.org/showlog.cgi?log=Try/1312534392.1312536112.31421.gz
in comment 78.  But I just remembered that Android can't handle focus
correctly yet.  So, can you just mark the test as needs-focus in the
reftest manifest?  That should make Android skip the test.

> About, when the blur is made inside double timeout, updateCurrentDictionary is
> not called.

Why?

> > Why is this change needed?
> 
> because when editor has an unkown language, there is no language set, and
> CheckWords returns a failure error.

Then can we just not attempt to do a spell check in that case?

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/303269

Title:
  Automatically select language for spell check based on user input

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/303269/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 303269]

[Ehsan-mozilla]

Comment on attachment 551030
patch v2.5

>diff -r 61bb2bb510c9 editor/libeditor/html/tests/test_bug484181.html
> function runTest() {
>   gMisspeltWords = ["haz", "cheezburger"];
>+  var edit = document.getElementById("edit");
>+  edit.focus();
>+  SimpleTest.executeSoon(function() {
>   is(isSpellingCheckOk(), true, "All misspellings before editing are 
> accounted for.");
> 
>-  var edit = document.getElementById("edit");
>   append(" becaz I'm a lolcat!");
>   SimpleTest.executeSoon(function() {
>   gMisspeltWords.push("becaz");
>   gMisspeltWords.push("lolcat");
>   is(isSpellingCheckOk(), true, "All misspellings after typing are accounted 
> for.");
> 
>   SimpleTest.finish();
>   });
>+  });
> }

Nit: could you please adjust the indentation here?

>diff -r 61bb2bb510c9 extensions/spellcheck/src/mozInlineSpellChecker.cpp
>+if (NS_FAILED(rv))
>+  continue;

Why is this change needed?

>diff -r 61bb2bb510c9 layout/reftests/editor/338427-2.html
>--- /dev/null  Thu Jan 01 00:00:00 1970 +
>+++ b/layout/reftests/editor/338427-2.html Thu Aug 04 23:34:57 2011 +0200
>@@ -0,0 +1,16 @@
>+
>+
>+

[Bug 303269]

[Ehsan-mozilla]

(In reply to Dão Gottwald [:dao] from comment #72)
> (In reply to comment #71)
> > ::: layout/reftests/editor/338427-2.html
> > @@ +6,5 @@
> > > +editor.focus();
> > > +window.setTimeout(function() {
> > > +editor.blur();
> > > +document.documentElement.className = '';
> > > +}, 1000); // XXX: 0 timeout is not enought to trigger a focus event.
> > 
> > Change this to:
> > 
> > setTimeout(function() {
> >   setTimeout(function() {
> > editor.blur();
> > document
> >   }, 0);
> > }, 0);
> > 
> > ::: layout/reftests/editor/338427-3.html
> > @@ +7,5 @@
> > > +editor.focus();
> > > +window.setTimeout(function() {
> > > +editor.blur();
> > > +document.documentElement.className = '';
> > > +}, 1000); // XXX: 0 timeout is not enought to trigger a focus event.
> > 
> > Same thing here.
> 
> Err, why isn't this using focus event listeners?

Because spellchecking doesn't happen on focus, we post an event to the
event loop and start the spellchecking when that event is processed.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/303269

Title:
  Automatically select language for spell check based on user input

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/303269/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 107247]

[Ehsan-mozilla]

(In reply to comment #18)
> Ehsan, does Neil's approach sound better to you? It does to me.

As I've said before, I don't understand the focus manager code, so if
Neil (Deakin) vouches on that, I'd be fine with Neil's approach.  I'm
assuming that he's going to address the todo items in comment 13 once
Neil reviews the focus manager change.  Also, Neil, can you check to see
if my test case passes successfully with your patch?

(In reply to comment #13)
> Note that even with caret browsing on, this code doesn't move focus in to a
> contenteditable region either. I don't know whether this is reasonable.

That's definitely not desired, but I tested it and we are doing the same
thing right now even without this patch, so I don't think we need to
worry about this problem here.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/107247

Title:
  Launchpad bug pages trigger caret browsing in Firefox and other Gecko
  browsers

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/107247/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 107247]

[Ehsan-mozilla]

(In reply to comment #20)
> (In reply to comment #19)
> > Is this a regression in 6? How widespread is this likely to be? We're just a
> > few days from the final Beta of 6.
> 
> This is not a regression in 6, from what Ehsan said on IRC, but e.g.
> planet.m.o is unusable using keys for several days now, due to this bug.

While that is true, this bug has existed for ages (I think from Firefox
3+) and there is no good reason why we should track it for Firefox 6...

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/107247

Title:
  Launchpad bug pages trigger caret browsing in Firefox and other Gecko
  browsers

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/107247/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 107247]

[Ehsan-mozilla]

(In reply to comment #25)
> I also noted that middle-clicking links on Planet Mozilla doesn't work.
> Same bug?

No, please file one?

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/107247

Title:
  Launchpad bug pages trigger caret browsing in Firefox and other Gecko
  browsers

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/107247/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 107247]

[Ehsan-mozilla]

(In reply to comment #23)
> Likely caused by the post that uses contenteditable.

Indeed.

*** This bug has been marked as a duplicate of bug 669026 ***

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/107247

Title:
  Launchpad bug pages trigger caret browsing in Firefox and other Gecko
  browsers

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/107247/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 303269]

[Ehsan-mozilla]

Comment on attachment 549230
patch v2.2

Review of attachment 549230:
-

r=me with the below comments fixed.

::: content/base/public/nsIContent.h
@@ +948,5 @@
> +   * Determing language. Look at the nearest ancestor element that has a lang
> +   * attribute in the XML namespace or is an HTML element and has a lang in
> +   * no namespace attribute.
> +   */
> +  void GetLang(nsAString& aResult) {

Oh, I almost forgot about this!  Make this a const method please?

::: layout/reftests/editor/338427-2.html
@@ +6,5 @@
> +editor.focus();
> +window.setTimeout(function() {
> +editor.blur();
> +document.documentElement.className = '';
> +}, 1000); // XXX: 0 timeout is not enought to trigger a focus event.

Change this to:

setTimeout(function() {
  setTimeout(function() {
editor.blur();
document
  }, 0);
}, 0);

::: layout/reftests/editor/338427-3.html
@@ +7,5 @@
> +editor.focus();
> +window.setTimeout(function() {
> +editor.blur();
> +document.documentElement.className = '';
> +}, 1000); // XXX: 0 timeout is not enought to trigger a focus event.

Same thing here.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/303269

Title:
  Automatically select language for spell check based on user input

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/303269/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 303269]

[Ehsan-mozilla]

(In reply to comment #66)
> > Can you add another test here for the dynamic change case?  You can put
> > lang="en-US" on the textarea, then upon load change it to "testing-XX", then
> > .focus() and .blur().
> 
> Actually, this does not work. Calling SetCurrentDictionary to an invalid
> language, does nothing. So an invalid language at startup (or first focus) 
> does
> not initialize spellchecker. But an invalid language after spellchecker has
> been initialized to a language does not reset it.

Hmm, well, nevermind those tests then, I guess.  :(

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/303269

Title:
  Automatically select language for spell check based on user input

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/303269/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 303269]

[Ehsan-mozilla]

Comment on attachment 548731
patch v2

Review of attachment 548731:
-

The patch looks very good, r=me on it with the comments below addressed.

Thanks a lot for working on this!

::: editor/composer/src/nsEditorSpellCheck.cpp
@@ +188,5 @@
>rv = mSpellChecker->SetDocument(tsDoc, PR_TRUE);
>NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // do not fail if UpdateCurrentDictionary fails because this method may
> +  // success later.

Nit: succeed

::: layout/reftests/editor/338427-ref.html
@@ +1,4 @@
> +
> +
> +
> +strangeimpossibleword

Technically, the -ref file is the reference rendering, and the other
file is the thing to be tested.  So, these two files should probably be
swapped.

::: layout/reftests/editor/reftest.list
@@ +62,5 @@
>  == 642800.html 642800-ref.html
>  == selection_visibility_after_reframe.html 
> selection_visibility_after_reframe-ref.html
>  != selection_visibility_after_reframe-2.html 
> selection_visibility_after_reframe-ref.html
>  == 672709.html 672709-ref.html
> +== 338427.html 338427-ref.html

Can you add another test here for the dynamic change case?  You can put
lang="en-US" on the textarea, then upon load change it to "testing-XX",
then .focus() and .blur().

Also, can you please add equivalent tests for contenteditable?

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/303269

Title:
  Automatically select language for spell check based on user input

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/303269/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 303269]

[Ehsan-mozilla]

(In reply to comment #59)
> The hard part there is xml:lang, since it needs to apply to all elements and
> not just the ones that do attribute mapping...

Then maybe we can defer this part to another bug.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/303269

Title:
  Automatically select language for spell check based on user input

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/303269/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 303269]

[Ehsan-mozilla]

(In reply to comment #54)
> Right; if the style property is good enough we could easily add a notification
> when it changes.

If we modify it to match the HTML5 spec, that would probably work fine.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/303269

Title:
  Automatically select language for spell check based on user input

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/303269/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 303269]

[Ehsan-mozilla]

(In reply to comment #56)
> (In reply to comment #55)
> > Created attachment 548559 [details] [review]
> > wip
> > 
> > wip patch:
> > it deals with ehsan suggestions except for the html editor handling (ie:
> > GetActiveEditingHost method).
> > What needs to be done is call GetActiveEditingHost at the right moment
> > (editor needs to be focused otherwise, result is wrong). ehsan suggested on
> > irc to make GetActiveEditingHost a [noscript,notxpcom] method of
> > nsIHTMLEditor.
> 
> There can be more than one contenteditable element per editor. Therefore
> getting this part done looks quite tricky. Computing editor lang cannot be 
> done
> during initialization. It should be done at spell checking time. Or, as 
> volkmar
> proposed in comment #35, at focus time.

Doing that on focus makes sense to me...

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/303269

Title:
  Automatically select language for spell check based on user input

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/303269/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 303269]

[Ehsan-mozilla]

(In reply to comment #52)
> We don't have an existing notification system that would work for that. 
> We'd need to add something.
> 
> That said, you only care about this for things that have frames, no?  Is
> there a reason that GetStyleVisibility()->mLanguage (which is somewhat
> different, in that I think it ignores Content-Language but does look at
> charset and ignores xml:lang, but we could fix those differences if we
> needed to) won't work for you?  That has automatic change notifications
> built in, sorta.

But we need to be notified when the language changes, in order to be
able to change our dictionary and trigger another spell check round.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/303269

Title:
  Automatically select language for spell check based on user input

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/303269/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 303269]

[Ehsan-mozilla]

(In reply to comment #50)
> > For dynamic changes, let's see if bz has any ideas how to handle them
> > efficiently
> 
> So the goal here is that you want to be notified when the language of some
> particular content node changes, right?

Yes.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/303269

Title:
  Automatically select language for spell check based on user input

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/303269/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 303269]

[Ehsan-mozilla]

Comment on attachment 547702
updated patch

Review of attachment 547702:
-

In general I like this patch.  I'm minusing because of the GetLanguage
part, but the rest generally looks good.

About testing strategies, you can load dictionaries from a directory
using the loadDictionariesFromDir API (see 
for an example).  But I would be fine with tests written without loading
custom dictionaries, if we use the same code for this and the :lang CSS
selector (see below).  Once we do that, we can get more unit tests for
the :lang selector (this is our only test for that now:
) and then we can trust that editor is going
to use the same code to retrieve the language name, which reduces the
need for loading additional dictionaries in our tests.  But we should
still get tests to make sure that the stuff in other languages is not
spell-checked by the editor.  You can use the existing spellcheck tests
in  as a sample for that.

Once we have this level of testing, we can just load some simple
dictionaries for some fictional languages (such as testing, testing-AB,
testing-AC) to make sure that they're not going to affect other tests in
the system, and use them to test the fallback language stuff.

For dynamic changes, let's see if bz has any ideas how to handle them
efficiently, but I think that we definitely want to handle them, because
it's the only way that a web page can signal us to change the spell
checking language.  But this can definitely be done in a follow-up bug.

Does this make sense?

::: editor/composer/src/nsEditorSpellCheck.cpp
@@ +251,4 @@
>  }
>}
>  
> +  // If we have not set editor, and editor has not a @lang, we try to get a

Nit: "If the editable element doesn't have a lang attribute, ..."

@@ +283,5 @@
>  NS_IMETHODIMP
> +nsEditorSpellCheck::GetLanguage(nsIEditor* aEditor, nsAString &aResult)
> +{
> +  nsCOMPtr rootElement;
> +  nsresult rv = aEditor->GetRootElement(getter_AddRefs(rootElement));

This only work correctly for the plaintext editor.  For HTML editor, you
need to call GetActiveEditingHost() (you should probably cast to
nsHTMLEditor manually :( ), and use the @lang attribute on that element
if it exists, and walk up the parent chain if it doesn't.

@@ +289,5 @@
> +
> +  nsCOMPtr rootContent = do_QueryInterface(rootElement);
> +  NS_ENSURE_TRUE(rootContent, NS_ERROR_FAILURE);
> +
> +  nsCOMPtr parent = rootContent->GetParent();

So, we have some code here  (also see the
GetLang function in that file) which implements the same thing for the
:lang CSS selector.  I think we should consolidate all this into an
nsIContent method named GetLanguage or something, and then we can simply
call this function in both places.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/303269

Title:
  Automatically select language for spell check based on user input

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/303269/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 801778]

[Ehsan-mozilla]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2b1eb891d7fe

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/801778

Title:
  Regression: Firefox 3.6.18 does not set cookie when talking to single
  letter hostname

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/801778/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 801778]

[Ehsan-mozilla]

I landed the m-c test on inbound.  Please do not mark the bug as FIXED
when you merge to central.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/801778

Title:
  Regression: Firefox 3.6.18 does not set cookie when talking to single
  letter hostname

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/801778/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 801778]

[Ehsan-mozilla]

The ubuntu folks want this badly.  Can we get the approval on this one
soon, please?

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/801778

Title:
  Regression: Firefox 3.6.18 does not set cookie when talking to single
  letter hostname

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/801778/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 539226]

[Ehsan-mozilla]

http://hg.mozilla.org/mozilla-central/rev/7c42f37e0284

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/539226

Title:
  firefox(-gnome-support) should be compiled with Gio support instead
  GnomeVFS

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 539226]

[Ehsan-mozilla]

http://hg.mozilla.org/projects/cedar/rev/7c42f37e0284

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/539226

Title:
  firefox(-gnome-support) should be compiled with Gio support instead
  GnomeVFS

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

[Bug 508969]

[Ehsan-mozilla]

*** This bug has been marked as a duplicate of bug 424627 ***

-- 
Strange behavior when pasting a text in a text box with mouse cursor on the 
edge of the text box and left mouse button pressed
https://bugs.launchpad.net/bugs/508969
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs