Last Comment Bug 748313 - queryCommandState("bold") doesn't recognize CSS if selection is collapsed
: queryCommandState("bold") doesn't recognize CSS if selection is collapsed
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: :Aryeh Gregor (working until September 2)
:
Mentors:
Depends on: 1140105 1141017 1159923
Blocks: richtext2 317093
  Show dependency treegraph
 
Reported: 2012-04-24 05:32 PDT by :Aryeh Gregor (working until September 2)
Modified: 2015-04-29 15:14 PDT (History)
2 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch part 1, v1 -- Remove unused argument to nsHTMLEditor::IsTextPropertySetByContent (8.87 KB, patch)
2012-05-02 04:05 PDT, :Aryeh Gregor (working until September 2)
ehsan: review+
Details | Diff | Splinter Review
Patch part 2, v1 -- Always try to use CSS values, even for collapsed selections (6.30 KB, patch)
2012-05-02 04:06 PDT, :Aryeh Gregor (working until September 2)
ehsan: review+
Details | Diff | Splinter Review

Description :Aryeh Gregor (working until September 2) 2012-04-24 05:32:44 PDT
See bug 317093 comment 12 -- I didn't fix the collapsed range case there.  Test-case:

data:text/html,<!DOCTYPE html>
<div contenteditable><strong>foo</strong></div>
<script>
getSelection().collapse(document.querySelector("strong").firstChild, 1);
document.body.textContent = document.queryCommandState("bold");
</script>

Outputs "false", should be "true".  Works with <b>, and works with non-collapsed selection.  Does not work with <span style="font-weight: bold">.  This is tested by RTE2-QS_B_STRONG-I-1_SC.  I basically just have to apply the same idea as bug 317093 part 4 to the collapsed-range case, using only CSS even in non-CSS mode.
Comment 1 :Aryeh Gregor (working until September 2) 2012-05-02 04:05:13 PDT
Created attachment 620247 [details] [diff] [review]
Patch part 1, v1 -- Remove unused argument to nsHTMLEditor::IsTextPropertySetByContent

Also made the method non-virtual while I was at it, because I saw no reason it should be virtual.  There are no callers in comm-central.
Comment 2 :Aryeh Gregor (working until September 2) 2012-05-02 04:06:44 PDT
Created attachment 620248 [details] [diff] [review]
Patch part 2, v1 -- Always try to use CSS values, even for collapsed selections

The new crashtest pass here is probably because the underlying bugs are no longer being triggered by that specific testcase, not because they're actually fixed.
Comment 3 :Ehsan Akhgari 2012-05-02 16:46:18 PDT
Comment on attachment 620248 [details] [diff] [review]
Patch part 2, v1 -- Always try to use CSS values, even for collapsed selections

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

::: editor/libeditor/html/nsHTMLEditorStyle.cpp
@@ +1020,5 @@
>      if (isCollapsed) {
>        range->GetStartContainer(getter_AddRefs(collapsedNode));
>        NS_ENSURE_TRUE(collapsedNode, NS_ERROR_FAILURE);
>        bool isSet, theSetting;
> +      nsString tOutString; //MJUDGE SCC NEED HELP

Please remove this comment.  :-)

@@ +1040,5 @@
> +      if (mHTMLCSSUtils->IsCSSEditableProperty(collapsedNode, aProperty,
> +                                               aAttribute) &&
> +          // Bug 747889: we don't support CSS for fontSize values
> +          (aProperty != nsEditProperty::font ||
> +           !aAttribute->EqualsLiteral("size"))) {

Nit: please reorder the two sides of &&, as the right side is much more likely to be true.
Comment 4 :Aryeh Gregor (working until September 2) 2012-05-03 00:12:08 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/76570252700f
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac00c792933e

(In reply to Ehsan Akhgari [:ehsan] from comment #3)
> Nit: please reorder the two sides of &&, as the right side is much more
> likely to be true.

Okay -- although if the goal is efficiency, don't we want the left side to be the one that's most likely to be *false*?
Comment 5 :Ms2ger (⌚ UTC+1/+2) 2012-05-03 01:20:35 PDT
(In reply to Aryeh Gregor from comment #4)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/76570252700f
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ac00c792933e
> 
> (In reply to Ehsan Akhgari [:ehsan] from comment #3)
> > Nit: please reorder the two sides of &&, as the right side is much more
> > likely to be true.
> 
> Okay -- although if the goal is efficiency, don't we want the left side to
> be the one that's most likely to be *false*?

Not with an or; if the left side is true, we don't need to evaluate the right side.
Comment 6 :Aryeh Gregor (working until September 2) 2012-05-03 03:52:37 PDT
He said the two sides of &&, so that's what I swapped.  The left side of the || here is a pointer comparison and the right side is a method call, so that should stay how it is no matter what from an efficiency perspective.
Comment 8 :Ehsan Akhgari 2012-05-04 08:58:38 PDT
FWIW, my goal was to minimize the number of times IsCSSEditableProperty is called.
Comment 9 :Ms2ger (⌚ UTC+1/+2) 2012-05-04 14:02:04 PDT
[And I can't read]
Comment 10 Mozilla RelEng Bot 2012-05-11 15:12:01 PDT
Autoland Patchset:
	Patches: 620247, 620248
	Branch: mozilla-central => try
Patch 620247 could not be applied to mozilla-central.
patching file editor/libeditor/html/nsHTMLEditRules.cpp
Hunk #1 FAILED at 7449
1 out of 1 hunks FAILED -- saving rejects to file editor/libeditor/html/nsHTMLEditRules.cpp.rej
patching file editor/libeditor/html/nsHTMLEditor.cpp
Hunk #1 FAILED at 3905
1 out of 1 hunks FAILED -- saving rejects to file editor/libeditor/html/nsHTMLEditor.cpp.rej
patching file editor/libeditor/html/nsHTMLEditor.h
Hunk #1 FAILED at 540
1 out of 1 hunks FAILED -- saving rejects to file editor/libeditor/html/nsHTMLEditor.h.rej
patching file editor/libeditor/html/nsHTMLEditorStyle.cpp
Hunk #1 FAILED at 296
Hunk #2 FAILED at 1034
Hunk #3 FAILED at 1105
Hunk #4 FAILED at 1123
Hunk #5 FAILED at 1146
5 out of 5 hunks FAILED -- saving rejects to file editor/libeditor/html/nsHTMLEditorStyle.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patchset could not be applied and pushed.
Comment 11 neil@parkwaycc.co.uk 2015-03-05 13:03:40 PST
Comment on attachment 620248 [details] [diff] [review]
Patch part 2, v1 -- Always try to use CSS values, even for collapsed selections

>+      if (mHTMLCSSUtils->IsCSSEditableProperty(collapsedNode, aProperty,
>+                                               aAttribute) &&
>+          // Bug 747889: we don't support CSS for fontSize values
>+          (aProperty != nsEditProperty::font ||
>+           !aAttribute->EqualsLiteral("size"))) {
>+        mHTMLCSSUtils->IsCSSEquivalentToHTMLInlineStyleSet(
>+          collapsedNode, aProperty, aAttribute, isSet, tOutString,
>+          COMPUTED_STYLE_TYPE);
>+        if (outValue) {
>+          outValue->Assign(tOutString);
>+        }
I've been looking into bug 1139524.
The first problem is that this now returns the CSS font family rather than the HTML font attribute, which has all the spaces removed after the commas. Since this is apparently correct behaviour for the font family style, bug 1139524 will deal with that in the front end.
However as well as the font menulist I also checked the main font submenu and discovered an unrelated regression also caused by this patch, which I will file shortly.
If you look at the equivalent code for a selection later on in nsHTMLEditorStyle.cpp the fifth parameter is initialised from aValue. Unfortunately here it is not, which means that querying for a particular font will always succeed on a collapsed selection.

Note You need to log in before you can comment on or make changes to this bug.