237228 - pref "layout.word_select.eat_space_to_next_word" can't be set by user in the profile's prefs.js

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Vaclav Dvorak]

The boolean preference layout.word_select.eat_space_to_next_word isn't settable
in the user's profile (prefs.js or user.js) because of a bug very similar to bug
232321. (Also, for the same reason the pref can't be changed in about:config.)
Patch coming up, hopefully.

Comment 1

[Vaclav Dvorak]

Attachment: patch, adapted from bug 232321's attachment 141174

Comment 2

[Vaclav Dvorak]

Comment on attachment 143686 [details] [diff] [review]
patch, adapted from bug 232321's attachment 141174 [details] [diff] [review]

Warning: patch by clueless newbie. OTOH, it's all stolen from bug 232321, so it
could be right.

BTW, cute bug of Buzilla in previous comment. :-)

Comment 3

[Boris Zbarsky [:bzbarsky]]

I'll try to take a look Saturday morning.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 143686 [details] [diff] [review]
patch, adapted from bug 232321's attachment 141174 [details] [diff] [review]

>Index: layout/html/base/src/nsTextFrame.cpp
>+nsTextFrame::WordSelectListener::Observe(nsISupports *aSubject,
>+                                                 const char *aTopic,
>+                                                 const PRUnichar *aData)

Fix the indent there.

> nsTextFrame::~nsTextFrame()
> {
>+  NS_IF_RELEASE(sWordSelectListener);

Er, no.  That's no good at all.  That will wipe out the listener any time a
text frame is destroyed.  We don't want that.

If you need to add a Shutdown() method of some sort that will get called at app
shutdown, go ahead and do so.

Comment 5

[Vaclav Dvorak]

Attachment: patch, version 2

There must be a less invasive way to do this...??

I moved the definition of class nsTextFrame from the .cpp to the .h, and just
enough #include's to make it compile. I added a Shutdown() method to the class.
The warning applies more than before.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I won't be able to get to this for at least two weeks....

Comment 7

[Vaclav Dvorak]

Comment on attachment 143916 [details] [diff] [review]
patch, version 2

OK, thanks anyway. Let's try roc then?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Moving nsTextFrame into nsTextFrame.h is unfortunate but I guess it's the right
thing to do.

However, nsAutoIndexBuffer doesn't need to be in nsTextFrame.h. nsTextFrame.h
just needs "struct nsAutoIndexBuffer;" and the actual definition can stay where
it is.

Ditto for TextStyle. The declaration of TextStyle in nsTextFrame.cpp will be
"struct nsTextFrame::TextStyle { ..."

Ditto for TextReflowData.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+#include "nsTextTransformer.h"
+#include "nsIRenderingContext.h"
+#include "nsIFontMetrics.h"
+#include "nsILookAndFeel.h"
+#include "nsITextContent.h"

Plus, I think these includes can be left out of nsTextFrame.h and replaced where
necessary by

class nsIRenderingContext;

etc

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+#define TEXT_BUF_SIZE 100

This can stay in nsTextFrame.cpp once nsAutoIndexBuffer is back in
nsTextFrame.cpp ... you get the idea.

Comment 11

[Vaclav Dvorak]

Attachment: patch v3, addressing roc's comments

This should do it. As long as I was meddling with this file, I also took the
liberty of correcting some whitespace inconsistencies and tab abuse. It appears
that at various times, the file was edited by people using tabs 8, 2, and even
4 spaces wide. I'll attach a diff -uw version that should be more readable.

Comment 12

[Vaclav Dvorak]

Attachment: patch v3, without whitespace changes

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+class nsFrame;

You need to #include "nsFrame.h" here because it's used as a superclass.

Did this compile?

Comment 14

[Vaclav Dvorak]

Attachment: patch v4, addressing roc's comment

Oops. It compiled only because the only files that include nsTextFrame.h also
include nsFrame.h. Thanks, Robert.

Comment 15

[Vaclav Dvorak]

Attachment: patch v4, without whitespace changes

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 144045 [details] [diff] [review]
patch v4, addressing roc's comment

looks ok to me!

Comment 17

[Vaclav Dvorak]

Robert, could you perhaps recommend a better choice for a reviewer? Thanks.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

dbaron is the right choice for reviewer.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 144045 [details] [diff] [review]
patch v4, addressing roc's comment

I haven't seen any good justification for why we want this to be fixed, and it
doesn't seem like it's worth this much code.

However, it could probably be done in much less code with the prefs stuff jst
landed a few days ago (with one big pref observer for all of content/layout). 
Perhaps with that it would be little enough code that the tiny benefit of
allowing users to set this (rather than just following platform conventions,
which is what it's for now) offsets the amount of code increase.

Comment 20

[Ginn Chen]

Attachment: smaller patch

smaller patch, just copy from bug 232321

Comment 21

[Ginn Chen]

Comment on attachment 158300 [details] [diff] [review]
smaller patch

oops, it's just duplicate of patch version 1. I'm sorry.

Comment 22

[Ginn Chen]

What about move it into nsEventStateManager?
add,
PRBool nsEventStateManager::GetWordSelectEatSpaceAfter()
void nsEventStateManager::ResetWordSelectEatSpaceAfter()

So we don't need add nsTextFrame.h.
And I think nsFrame::GetFrameFromDirection need this setting.
I have a patch for bug 256252, which has this requirement.
I just workaround it, change aPos->mEatingWS to PR_FALSE and change it back 
after calling nsFrame::GetFrameFromDirection. It's a little ugly.

Comment 23

[Ginn Chen]

David, it is important for blind people to have this setting on linux.
For blind people, they need ctrl+right to go to beginning of the next word.
Because, if cursor goes to end of the word, the screen reader may not work 
correctly.

We must get it fixed to make Mozilla accessible on linux.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 158300 [details] [diff] [review]
smaller patch

you definitely don't want to destroy this observer every time a text frame is
destroyed

Comment 25

[Ginn Chen]

Attachment: patch, use nsTextTransformer's PrefCallback

add GetWordSelectEatSpaceAfter, GetWordSelectStopAtPunctuation to
nsTextTransformer

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 158360 [details] [diff] [review]
patch, use nsTextTransformer's PrefCallback

>+PRBool nsTextTransformer::sWordSelectEatSpaceAfter = PR_FALSE;

This should default to true for consistency with the current code.


Also, nsTextTransformer::Shutdown should remove both pref callbacks.

Comment 27

[Ginn Chen]

(In reply to comment #26)
> (From update of attachment 158360 [details] [diff] [review])
> >+PRBool nsTextTransformer::sWordSelectEatSpaceAfter = PR_FALSE;
> This should default to true for consistency with the current code.
On unix, the default values for these two settings are PR_FALSE;
On windows, they're PR_TRUE;
I tested on windows, it works well.
I think we don't want to set default value of sWordSelectEatSpaceAfter to 
PR_TRU, but set sWordSelectStopAtPunctuation to PR_FALSE.

> Also, nsTextTransformer::Shutdown should remove both pref callbacks.
In bug 240543. Hiding some nsIPref* API bloatyness in nsContentUtils.
"NS_IF_RELEASE(sWordSelectListener);" was removed from ShutDown();
Do we still need it?

Comment 28

[Ginn Chen]

Attachment: patch, add UnregisterPrefCallback

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 158389 [details] [diff] [review]
patch, add UnregisterPrefCallback

I was suggesting leaving the default value in case something somewhere relied
on what happened when some platform-specific part of all.js didn't set the
pref.

Comment 30

[Kyle Yuan]

fixed.