Closed Bug 748313 Opened 12 years ago Closed 12 years ago

queryCommandState("bold") doesn't recognize CSS if selection is collapsed

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla15

People

(Reporter: ayg, Assigned: ayg)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Patch part 1, v1 -- Remove unused argument to nsHTMLEditor::IsTextPropertySetByContent
8.87 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Patch part 2, v1 -- Always try to use CSS values, even for collapsed selections
6.30 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review 
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.
Flags: in-testsuite+
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.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attachment #620247 - Flags: review?(ehsan)
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.
Attachment #620248 - Flags: review?(ehsan)
Whiteboard: [autoland-try:-u all]
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]
Attachment #620247 - Flags: review?(ehsan) → review+
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.
Attachment #620248 - Flags: review?(ehsan) → review+
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*?
Target Milestone: --- → mozilla15
(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.
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.
https://hg.mozilla.org/mozilla-central/rev/76570252700f
https://hg.mozilla.org/mozilla-central/rev/ac00c792933e
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [autoland-in-queue]
FWIW, my goal was to minimize the number of times IsCSSEditableProperty is called.
[And I can't read]
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 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.
Depends on: 1140105
Depends on: 1141017
Depends on: 1159923
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: