1141017 - Querying for the current font sometimes returns "sans-serif" but doesn't return "serif" or "monospace"

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[[e-mail]]

[See bug 1139524 comment #15] The query font code special-cases "serif" and "monospace" because they are pseudo-families. However it does not special-case "sans-serif" and bug 748313 seems to have started tripping over this.

Comment 1

[[e-mail]]

Attachment: I shot the serif

But I didn't shoot the sans-serif...

I'm still writing a test for bug 1140105 but the good news is that once that's finished it shouldn't take so long to write a test for this patch.

Comment 2

[Jorg K (CEST = GMT+2)]

Thanks for taking care of the "sans-serif".

Now what about the second problem, the "comma space"? We need a patch for that.
My way by fixing the XUL or your way by removing the space on the fly?

Comment 3

[Jorg K (CEST = GMT+2)]

Damn, forget that comment, it should have gone to bug 1139524.

Comment 4

[(no longer active)]

Comment on attachment 8574618 [details] [diff] [review]
I shot the serif

Review of attachment 8574618 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!  This needs a test as well.

Comment 5

[[e-mail]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc155e427213 (with test)

Comment 6

[Carsten Book [:Tomcat]]

sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=202f81536f5e since one of this changes caused 

https://treeherder.mozilla.org/logviewer.html#?job_id=7522355&repo=mozilla-inbound

Comment 7

[Jorg K (CEST = GMT+2)]

Something fishy with this test. It compares for "sans-serif".
Why is "sans-serif" valid to return, but not "serif"?

Comment 8

[[e-mail]]

(In reply to Jorg K from comment #7)
> Something fishy with this test. It compares for "sans-serif".
> Why is "sans-serif" valid to return, but not "serif"?

If you run the original tests, "serif" and "monospace" fail as well, but I assume that versions used by treeherder have had the failures ignored in some way.

Comment 9

[(no longer active)]

(In reply to [e-mail] from comment #5)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/dc155e427213 (with
> test)

As far as I can tell, we do not run anything from editor/libeditor/TextEditorTest.cpp.  Please ask for review next time.

(In reply to [e-mail] from comment #8)
> (In reply to Jorg K from comment #7)
> > Something fishy with this test. It compares for "sans-serif".
> > Why is "sans-serif" valid to return, but not "serif"?
> 
> If you run the original tests, "serif" and "monospace" fail as well, but I
> assume that versions used by treeherder have had the failures ignored in
> some way.

It could be that we list them as known failures.  See <https://dxr.mozilla.org/mozilla-central/source/dom/imptests/failures/editing/conformancetest/test_runtest.html.json>.  But you need to make sure that these failures are indeed OK to ignore.

Comment 10

[[e-mail]]

(In reply to Ehsan Akhgari from comment #9)
> (In reply to comment #8)
> > (In reply to Jorg K from comment #7)
> > > Something fishy with this test. It compares for "sans-serif".
> > > Why is "sans-serif" valid to return, but not "serif"?
> > 
> > If you run the original tests, "serif" and "monospace" fail as well, but I
> > assume that versions used by treeherder have had the failures ignored in
> > some way.
> 
> It could be that we list them as known failures.  See
> <https://dxr.mozilla.org/mozilla-central/source/dom/imptests/failures/
> editing/conformancetest/test_runtest.html.json>.  But you need to make sure
> that these failures are indeed OK to ignore.

Do you know what the format of that file is? It does seem to mention a large number of sans-serif tests already.

Comment 11

[(no longer active)]

(In reply to [e-mail] from comment #10)
> (In reply to Ehsan Akhgari from comment #9)
> > (In reply to comment #8)
> > > (In reply to Jorg K from comment #7)
> > > > Something fishy with this test. It compares for "sans-serif".
> > > > Why is "sans-serif" valid to return, but not "serif"?
> > > 
> > > If you run the original tests, "serif" and "monospace" fail as well, but I
> > > assume that versions used by treeherder have had the failures ignored in
> > > some way.
> > 
> > It could be that we list them as known failures.  See
> > <https://dxr.mozilla.org/mozilla-central/source/dom/imptests/failures/
> > editing/conformancetest/test_runtest.html.json>.  But you need to make sure
> > that these failures are indeed OK to ignore.
> 
> Do you know what the format of that file is? It does seem to mention a large
> number of sans-serif tests already.

I'm not 100% sure.  IIRC these are generated by dom/imptests/parseFailures.py, by basically running the tests, looking at the failure messages and adding them in the right format to the corresponding json file.

But the more important question is: do you expect these failures with your patch?

These tests are defined in data.js.  That file basically defines a giant array of tests and each element is run by <https://dxr.mozilla.org/mozilla-central/source/dom/imptests/editing/tests.js#5538>.  From there you can reconstruct a small test case that examines some of these places where we are changing the behavior and you can test the results before and after your patch.  It could be that you're indeed fixing some of these failures which we have marked as expected, but IIRC the error reporting in these tests is very poor.

Comment 12

[[e-mail]]

I don't feel like touching those tests. Aryeh might even want to remove the existing special cases, so I'm probably better off working around this in the UI instead.

Comment 13

[Jorg K (CEST = GMT+2)]

Well, it would be good to do something *consistent*.
I've just raised bug 1148330 to cover the hassle we have with the inconsistent behaviour.
I don't know why "monospace" and "serif" are suppressed. If they weren't then the Linux behaviour would be better.

Comment 14

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #11)
> It could be that you're indeed fixing
> some of these failures which we have marked as expected, but IIRC the error
> reporting in these tests is very poor.

It's actually very useful *if* you know what it means.  :)  treeherder isn't loading for me, but if you paste the failure lines I should be able to tell you what's going on.

Comment 15

[Jorg K (CEST = GMT+2)]

Hi Aryeh, good to meet you on the next font related bug ;-)
I'm not sure what doesn't work for you, this link works for me:
https://treeherder.mozilla.org/logviewer.html#?job_id=7522355&repo=mozilla-inbound

The tests that fail are like:
03:34:56 INFO - 1537 INFO TEST-UNEXPECTED-FAIL | dom/imptests/editing/conformancetest/test_runtest.html | [["stylewithcss","true"],["fontname","sans-serif"]] "<p>[foo</p> <p>bar]</p>" queryCommandValue("fontname") after - [["stylewithcss","true"],["fontname","sans-serif"]] "<p>[foo</p> <p>bar]</p>" queryCommandValue("fontname") after: assert_equals: Wrong result returned expected "sans-serif" but got ""

So, expected "sans-serif" but got "", which is very much to be expected when sans-serif is suppressed.

Now the more important question is: Why was "serif" and "monospace" suppressed in the first place?
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLCSSUtils.cpp#1172
Blame says nothing, it's been like this since 2007. Do you know?

Comment 16

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

The test looks right to me.  Why should querying the command value not return "sans-serif"?  In general, I would say commands should return the current computed value, irrespective of whether it's the default value or applied by elements.  The use-case I know of for queryCommandValue() is so that a font-name drop-down in a WYSIWYG editor can tell the user what the current font is, and that should tell them the current font no matter how it got there.  Likewise, queryCommandValue("fontSize") should return "3", etc.  If Firefox doesn't behave that way, I think that's a bug.

Unless I'm missing something, I think we should take the opposite patch -- remove the special case for monospace and serif.

Comment 17

[Jorg K (CEST = GMT+2)]

Oops, I'd like to see what happens if we resurrect the serif and the monospace as per comment #16:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a21b67c98a4

As we've seen, serif and monospace are on the menus in TB on Linux, so why suppress information that we have? Refer to bug 1148330.

Comment 18

[[e-mail]]

(In reply to Jorg K from comment #17)
> As we've seen, serif and monospace are on the menus in TB on Linux

They actually exist as low-level font names on Linux, but as far as I know, they don't relate to the CSS font families of the same name.

Comment 19

[[e-mail]]

OK, so the point of those fonts is so that on Linux you can map Thunderbird's serif/sans-serif/monospace font to the Linux system default fonts in Options.

But of course when you select those fonts in the popup you actually get the font from Thunderbird's options, which could be different from the system default.

Comment 20

[Jorg K (CEST = GMT+2)]

I had a long discussion with Aceman the other day over in bug 1139524 comment #70 onwards. He didn't understand why we wanted to suppress "sans-serif" since this is the font he uses all the time. He is of the opinion that creating messages with <font face="sans-serif"> is a useful thing to have. He wasn't aware that "serif" is never returned and when he chooses "serif" it jumps to "Variable Width".

I don't mind either way, but I don't want to give up the investigation until we have a consistent base to build on. Either ignore all or resurrect what is currently ignored. If this is too much hassle, I can still give up later ;-)

Anyway, we spawned off to more (client) bugs: bug 1148330 and bug 1148790.

For TB 38 we will land 1139524 and 1148330. When this bug here is fixed or wont-fixed we can take the matter further.

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: Patch to resurrect serif and monospace (test changes still missing)

OK, the test results look good.
In dom/imptests/editing/conformancetest/test_runtest.html
we get a lot of TEST-UNEXPECTED-PASS, so I guess that's good.

In editor/libeditor/tests/test_bug408231.html we get a failure since now "serif" is returned but not expected.

If we all agree, I'll fix the tests and we're done here. In the TB client we can worry about the consequences.

Comment 22

[Jorg K (CEST = GMT+2)]

Attachment: List of 114 unexpected passes

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: Patch to resurrect serif and monospace (conformancetest test changes still missing)

editor/libeditor/tests/test_bug408231.html is fixed in this patch.

However, we have 114 unexpected passes (see attachment 8585138 [details]) and I'm not sure how to mark them as not longer expected to fail.

Somewhere in dom/imptests/failures/editing/conformancetest/test_runtest.html.json perhaps?

A bit of a tedious job to identify the 114 lines that need removing/changing. I'm also not sure why my change is causing these, since they are all about "sans-serif" whose behaviour I haven't changed.

Comment 24

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8585139 [details] [diff] [review]
Patch to resurrect serif and monospace (conformancetest test changes still missing)

Review of attachment 8585139 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Jorg K from comment #21)
> OK, the test results look good.
> In dom/imptests/editing/conformancetest/test_runtest.html
> we get a lot of TEST-UNEXPECTED-PASS, so I guess that's good.

It's good if we agree with the tests.  I do (which is why I wrote them that way :) ), but Ehsan has to decide.

(In reply to Jorg K from comment #23)
> However, we have 114 unexpected passes (see attachment 8585138 [details])
> and I'm not sure how to mark them as not longer expected to fail.
> 
> Somewhere in
> dom/imptests/failures/editing/conformancetest/test_runtest.html.json perhaps?
> 
> A bit of a tedious job to identify the 114 lines that need
> removing/changing.

There are two ways to do this:

1) Edit testharnessreport.js to set the dumpFailures flag to true, then run the tests and save the output to a log file, then run the parseFailures.py script to regenerate the file.

2) A simple shell script should do it for you.  Maybe something like:

cat file_with_failures | sed 's/.*test_runtest.html | //;s/"/\\\\"/;s/[/\\[/' | while read line; do sed -i "!$line!d" test_runtest.html.json; done

but I didn't test that and you might need to debug it.  If you understand what it does, it's probably the fastest way.  If this is too annoying for you, I can do it for you.

> I'm also not sure why my change is causing these, since
> they are all about "sans-serif" whose behaviour I haven't changed.

The tests run queryCommandIndeterm/State/Value both before and after the commands are executed.  These unexpected passes are from *before* the command execution (notice the word "before" at the end of the test name), so it should be serif, since that's the default font.  After the command execution the text is sans-serif, so the check afterwards passed even without your patch.  The reason they're all sans-serif is because that's the font I used for generic fontName setting.


The patch looks reasonable to me.

Comment 25

[Jorg K (CEST = GMT+2)]

(In reply to :Aryeh Gregor from comment #24)
> If this is too annoying for you, I can do it for you.

If you have time, it would be nice if you could do it and then push it to the try server so we can get Ehsan's approval as soon as possible. Please let me know so we don't do the work twice.

I'd really like to know whether we're going to resurrect the "serif", since I want to cater for the case in TB 38, which is pretty much in the next 24 hours (even is the core change is coming in mozilla39).

Comment 26

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I'll do that.

Comment 27

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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

Comment 28

[Jorg K (CEST = GMT+2)]

Thank you!! Quite a job ;-)

Comment 29

[Jorg K (CEST = GMT+2)]

Attachment: Aryeh Gregor's changes to the conformance test. Thanks!!

@Aryeh: Can you please check I got this right. I extracted it from the changeset.

Comment 30

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8585317 [details] [diff] [review]
Aryeh Gregor's changes to the conformance test. Thanks!!

These will need to be folded into the patch that changes the code, so that we don't have any commits that fail tests.  If you push multiple changes where an intermediate commit fails tests, it won't be caught by treeherder, but it might annoy people who are later bisecting to find regressions.  You don't have to do this, though -- whoever pushes the patch to inbound should be able to fold them for you.

Comment 31

[Jorg K (CEST = GMT+2)]

I don't push anything here ;-) I also didn't want to sell Aryeh's works as mine.
So Ehsan, if you approve, can you please push the two patches together. Thank you.

Comment 32

[(no longer active)]

This bug has gotten super confusing, and I no longer can follow what problem we're trying to solve, and why the patches do the right thing (note that I have no idea what the Thunderbird side issue is.)  Can someone please submit a single patch that contains all of the necessary changes, with a description of the issue that the patch is solving?  Thanks!

Comment 33

[(no longer active)]

Comment on attachment 8585139 [details] [diff] [review]
Patch to resurrect serif and monospace (conformancetest test changes still missing)

(Not sure what exactly I need to review here.)

Comment 34

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

The issue is -- if the current font is (generic) monospace or serif, queryCommandValue("fontName") currently returns "", but if it's sans-serif, it returns "sans-serif".  This does not appear to make much sense.  Initially we had patches to change it to return "" for sans-serif also, but I argued in comment 16 that we should change the other way, and return "monospace" and "serif" instead of "".  The latter two patches (the ones I have feedback+ on), when combined, implement this second suggestion, and have a green try run here:

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

Comment 35

[(no longer active)]

Thanks that makes sense.  But please submit a single patch for review and checkin.

Comment 36

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(Jorg, you can use "hg qfold" to combine additional patches into the current patch.  Make sure the first patch is on top of your queue, and the second one is not, and then do "hg qfold second-patch".)

Comment 37

[Jorg K (CEST = GMT+2)]

Attachment: Patch to resurrect serif and monospace (conformance test changes by Aryeh Gregor)

feedback+ by Aryeh Gregor carried forward from superseded patches.

Comment 38

[Carsten Book [:Tomcat]]

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

Comment 39

[Ryan VanderMeulen [:RyanVM]]

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