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

RESOLVED FIXED in Firefox 40

Status

()

Core
Editor
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: Jorg K (GMT+2))

Tracking

Trunk
mozilla40
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

2 years ago
[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.
(Reporter)

Comment 1

2 years ago
Created attachment 8574618 [details] [diff] [review]
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.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #8574618 - Flags: review?(ehsan)
(Assignee)

Comment 2

2 years ago
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?
Flags: needinfo?(neil)
(Assignee)

Comment 3

2 years ago
Damn, forget that comment, it should have gone to bug 1139524.
Flags: needinfo?(neil)
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.
Attachment #8574618 - Flags: review?(ehsan) → review+
(Reporter)

Comment 5

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc155e427213 (with test)
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
(Assignee)

Comment 7

2 years ago
Something fishy with this test. It compares for "sans-serif".
Why is "sans-serif" valid to return, but not "serif"?
(Reporter)

Comment 8

2 years ago
(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.
(In reply to neil@parkwaycc.co.uk 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 neil@parkwaycc.co.uk 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.
(Assignee)

Updated

2 years ago
Blocks: 1139524
(Reporter)

Comment 10

2 years ago
(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.
(Assignee)

Updated

2 years ago
Blocks: 1142879
(In reply to neil@parkwaycc.co.uk 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.
(Reporter)

Comment 12

2 years ago
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.
(Assignee)

Comment 13

2 years ago
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.
(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.
(Assignee)

Comment 15

2 years ago
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?
(Assignee)

Updated

2 years ago
Flags: needinfo?(ayg)
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.
Flags: needinfo?(ayg)
(Reporter)

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
(Assignee)

Comment 17

2 years ago
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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Assignee)

Updated

2 years ago
Assignee: neil → mozilla
(Reporter)

Comment 18

2 years ago
(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.
(Reporter)

Comment 19

2 years ago
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.
(Assignee)

Comment 20

2 years ago
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.
(Assignee)

Updated

2 years ago
Summary: Querying for the current font sometimes returns "sans-serif" → Querying for the current font sometimes returns "sans-serif" but doesn't return "serif" or "monospace"
(Assignee)

Comment 21

2 years ago
Created attachment 8585098 [details] [diff] [review]
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.
Attachment #8585098 - Flags: feedback?(ayg)
(Assignee)

Comment 22

2 years ago
Created attachment 8585138 [details]
List of 114 unexpected passes
(Assignee)

Comment 23

2 years ago
Created attachment 8585139 [details] [diff] [review]
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.
Attachment #8585098 - Attachment is obsolete: true
Attachment #8585098 - Flags: feedback?(ayg)
Flags: needinfo?(ehsan)
Attachment #8585139 - Flags: feedback?(ayg)
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.
Attachment #8585139 - Flags: feedback?(ayg) → feedback+
(Assignee)

Comment 25

2 years ago
(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).
I'll do that.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e2987969bb7
(Assignee)

Comment 28

2 years ago
Thank you!! Quite a job ;-)
(Assignee)

Updated

2 years ago
Attachment #8585139 - Flags: review?(ehsan)
(Assignee)

Comment 29

2 years ago
Created attachment 8585317 [details] [diff] [review]
Aryeh Gregor's changes to the conformance test. Thanks!!

@Aryeh: Can you please check I got this right. I extracted it from the changeset.
Attachment #8585317 - Flags: review?(ehsan)
Attachment #8585317 - Flags: feedback?(ayg)
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.
Attachment #8585317 - Flags: feedback?(ayg) → feedback+
(Assignee)

Comment 31

2 years ago
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.
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!
Flags: needinfo?(ehsan)
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.)
Attachment #8585139 - Flags: review?(ehsan)
Attachment #8585317 - Flags: review?(ehsan)
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
Flags: needinfo?(ehsan)
Thanks that makes sense.  But please submit a single patch for review and checkin.
Flags: needinfo?(ehsan)
(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".)
(Assignee)

Comment 37

2 years ago
Created attachment 8585640 [details] [diff] [review]
Patch to resurrect serif and monospace (conformance test changes by Aryeh Gregor)

feedback+ by Aryeh Gregor carried forward from superseded patches.
Attachment #8574618 - Attachment is obsolete: true
Attachment #8585139 - Attachment is obsolete: true
Attachment #8585317 - Attachment is obsolete: true
Attachment #8585640 - Flags: review?(ehsan)
Attachment #8585640 - Flags: feedback+
Attachment #8585640 - Flags: review?(ehsan) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/a474e9f2c5cd
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a474e9f2c5cd
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox40: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.