bug#43218: EWW handles default answer incorrectly when changing a select

bug#43218: EWW handles default answer incorrectly when changing a select

[Nicolas Graner]

When you type <RET> on a "select" input in EWW, you are prompted for a
new value in the minibuffer. If you just type <RET> to the prompt, the
selected value should remain unchanged. Instead, the value is replaced
with an empty string, which effectively make the select disappear.

Explanation: the function eww-change-select calls:
  (completing-read "Change value: " options nil 'require-match)
without providing a default value, and uses its return value unconditionally. If the user just types <RET>, that return value is an empty string.

To fix it, the code should either pass the currently selected value as the DEF argument to completing-read, or ignore the return value when it is empty.

Nicolas

bug#43218: EWW handles default answer incorrectly when changing a select

[Lars Ingebrigtsen]

Nicolas Graner <nicolas.graner@universite-paris-saclay.fr> writes:

> To fix it, the code should either pass the currently selected value as
> the DEF argument to completing-read, or ignore the return value when
> it is empty.

Yup.  Either would work...  I chose to supply a default value.  Fix in
Emacs 28.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43218: EWW handles default answer incorrectly when changing a select

[Nicolas Graner]

Lars Ingebrigtsen <larsi@gnus.org> a écrit le 05/09/2020 à 15h51 :
> Either would work...  I chose to supply a default value.

The fix works in most situations but not when several options have the
same value.
Example:

--------------------------------------------------
<form method="post">
<select name="a">
<option value="1">one</option>
<option value="1">two</option>
</select>
</form>
--------------------------------------------------

Activate the select and change its value to 'two'.
Activate it again and answer <RET> to the prompt for value: the
displayed value returns to 'one'.

This is not critical as this situation is rare and the value sent to the
server will be correct anyway, but it is confusing to the user who does
not usually know the value of each option.

Nicolas

bug#43218: EWW handles default answer incorrectly when changing a select

[Lars Ingebrigtsen]

Nicolas Graner <nicolas.graner@universite-paris-saclay.fr> writes:

> Activate the select and change its value to 'two'.
> Activate it again and answer <RET> to the prompt for value: the
> displayed value returns to 'one'.
>
> This is not critical as this situation is rare and the value sent to the
> server will be correct anyway, but it is confusing to the user who does
> not usually know the value of each option.

That's true...  there's also a general problem with how the values are
selected: We're doing a completing-read over the display names, and then
mapping that back to the values.  But this is perfectly valid:

<select name="a">
<option value="1">one</option>
<option value="2">one</option>
</select>

But there's no way to select the second value in eww.

Unfortunately, the Emacs primitives for prompting are very
text-oriented, and don't allow putting properties on the values we're
completing over, not even with:

---
minibuffer-allow-text-properties is a variable defined in ‘src/minibuf.c’.
Its value is nil

  Probably introduced at or before Emacs version 20.

Documentation:
Non-nil means ‘read-from-minibuffer’ should not discard text properties.
This also affects ‘read-string’, but it does not affect ‘read-minibuffer’,
‘read-no-blanks-input’, or any of the functions that do minibuffer input
with completion; they always discard text properties.
---

I wonder what the logic behind this is?  And I also have a vague feeling
I've asked this before...

Perhaps Stefan knows and or remembers.  :-)

So the test case is:

(let ((minibuffer-allow-text-properties t))
  (completing-read "Foo: " (list (propertize "foo" 'data 'bar))
                   nil 'require-match))
=> "foo"

instead of #("foo" 0 3 (data bar))

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43218: EWW handles default answer incorrectly when changing a select

[Stefan Monnier]

> <select name="a">
> <option value="1">one</option>
> <option value="2">one</option>
> </select>
>
> But there's no way to select the second value in eww.
>
> Unfortunately, the Emacs primitives for prompting are very
> text-oriented, and don't allow putting properties on the values we're
> completing over, not even with:

Not sure how the prompting gets involved in this problem: if you decide
to let the user choose only based on the text (i.e. "one" vs "one"),
then there's not much the prompting can do to help: the minibuffer lets
the users enter a string and that's that.
If they type "one" which one of the two should you pick?


        Stefan

bug#43218: EWW handles default answer incorrectly when changing a select

[Drew Adams]

> > Activate the select and change its value to 'two'.
> > Activate it again and answer <RET> to the prompt for value: the
> > displayed value returns to 'one'.
> >
> > This is not critical as this situation is rare and the value sent to the
> > server will be correct anyway, but it is confusing to the user who does
> > not usually know the value of each option.
> 
> That's true...  there's also a general problem with how the values are
> selected: We're doing a completing-read over the display names, and then
> mapping that back to the values.  But this is perfectly valid:
> 
> <select name="a">
> <option value="1">one</option>
> <option value="2">one</option>
> </select>
> 
> But there's no way to select the second value in eww.
> 
> Unfortunately, the Emacs primitives for prompting are very
> text-oriented, and don't allow putting properties on the values we're
> completing over, not even with:
> 
> ---
> minibuffer-allow-text-properties is a variable defined in ‘src/minibuf.c’.
> Its value is nil
> 
>   Probably introduced at or before Emacs version 20.
> 
> Documentation:
> Non-nil means ‘read-from-minibuffer’ should not discard text properties.
> This also affects ‘read-string’, but it does not affect ‘read-minibuffer’,
> ‘read-no-blanks-input’, or any of the functions that do minibuffer input
> with completion; they always discard text properties.
> ---
> 
> I wonder what the logic behind this is?  And I also have a vague feeling
> I've asked this before...
> 
> Perhaps Stefan knows and or remembers.  :-)
> 
> So the test case is:
> 
> (let ((minibuffer-allow-text-properties t))
>   (completing-read "Foo: " (list (propertize "foo" 'data 'bar))
>                    nil 'require-match))
> => "foo"
> 
> instead of #("foo" 0 3 (data bar))

Huge apologies for jumping in here, possibly
missing the point completely, and probably
not providing much that helps operationally.

But I think I know what you're talking about.

If so, first, note this:

(let ((minibuffer-allow-text-properties t))
  (read-from-minibuffer
    "Foo: " nil nil nil nil
    (propertize "foo" 'data 'bar)))

Use `M-n RET'.  The doc tells you that it only
returns a propertized string that was in the
_minibuffer_ when you hit RET.

That's not the case for a chosen
`completing-read' candidate.  As you quoted
from the doc string:

> the functions that do minibuffer input with
> completion ... always discard text properties.

Second, I can speak a bit to how Icicles deals
with this sort of thing.

I tried (more than once) to get vanilla Emacs
to let you use propertized strings as completion
candidates, both (1) for display in *Completions*
and (2) as returned value for `completing-read'.

I see that in the Icicles doc I mention that
in 2007 RMS agreed to fix Emacs to provide #1,
but AFAIK that never happened.

Icicles redefines `display-completion-alist'
so that it does display propertized candidates.

And Icicles redefines `completing-read' so  it
returns a propertized display-candidate string
that you choose.

You can do more.  But already those two simple
additions go a long way.

Wrt the problem I think you raise above, which
is dealing with multiple candidates that have
the same display string but are associated with
different "values" in some sense (the simplest
being the cdr of an alist cons whose car is the
display candidate):

1. When you cycle among candidates or use mouse-2
to choose one, there's the notion of "current
candidate", so Icicles knows which alist element
you choose (not just which display string).

2. If you bind `icicle-whole-cand-as-text-prop-p'
to non-nil then an entire alist element is put
on its car (the display string) as a property, so
when `completing-read' returns that string the
full candidate (the alist element) can be recovered.
This takes care of distinguishing element
("foo" . 42) from element ("foo" 5 4 3).

#2 is just this:

(let ((text-cand  (copy-sequence (car cand))))
  (put-text-property 0 (length text-cand)
                     'icicle-whole-candidate
                     (cdr cand) text-cand)
  (setcar cand text-cand)
  text-cand)

Then:

(and icicle-whole-candidate-as-text-prop-p
     (get-text-property
       0 'icicle-whole-candidate string))

(I use the same "trick" in Bookmark+ to handle
different bookmarks that have the same name.)

Here are two doc pages that discuss this a bit:

https://www.emacswiki.org/emacs/Icicles_-_Programming_with_Fancy_Candidates#fancy_candidates

https://www.emacswiki.org/emacs/Icicles_-_Candidates_with_Text_Properties

bug#43218: EWW handles default answer incorrectly when changing a select

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Not sure how the prompting gets involved in this problem: if you decide
> to let the user choose only based on the text (i.e. "one" vs "one"),
> then there's not much the prompting can do to help: the minibuffer lets
> the users enter a string and that's that.
> If they type "one" which one of the two should you pick?

Herp derp, I eat paste, as they say.

Yeah, that makes no sense.  I was kinda sorta thinking about how ido
presents choices, but that's pretty much irrelevant in this case.

I think the select thing in eww has to be rewritten to allow the choices
in a totally different way (and that should also help with <select
multiple>, probably).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43218: EWW handles default answer incorrectly when changing a select

[Nicolas Graner]

Lars Ingebrigtsen <larsi@gnus.org wrote on 06/09/2020 at 13h48 :
> I think the select thing in eww has to be rewritten to allow the choices
> in a totally different way (and that should also help with <select
> multiple>, probably).

I agree. IMHO options should be presented like a list of checkboxes in a
dedicated temporary buffer. In addition to multiple selections,
advantages include:

* no mabiguity if several options have same value and/or text
* can handle option with empty text
* can handle disabled options
* present options in the order intended, not alphabetized as in minibuffer completions
* search commands are available (userful if there are many options)

Couldn't a large part of the code for checkboxes/radio buttons be reused?

Nicolas

bug#43218: EWW handles default answer incorrectly when changing a select

[Lars Ingebrigtsen]

Nicolas Graner <nicolas.graner@universite-paris-saclay.fr> writes:

> I agree. IMHO options should be presented like a list of checkboxes in a
> dedicated temporary buffer. In addition to multiple selections,
> advantages include:
>
> * no mabiguity if several options have same value and/or text
> * can handle option with empty text
> * can handle disabled options
> * present options in the order intended, not alphabetized as in
> minibuffer completions
> * search commands are available (userful if there are many options)

Those are good points...  Dropping into a recursive-edit (in a new
temporary buffer) can be somewhat confusing for the user, though.  (epa
does something similar when selecting a public key for encryption, and
it's less than perfect from a usability standpoint.)

> Couldn't a large part of the code for checkboxes/radio buttons be reused?

Probably.

It'd be nice if Emacs had a real drop-down in-buffer menu -- that's how
other browsers present a select box.  Or has Emacs gotten that lately?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43218: EWW handles default answer incorrectly when changing a select

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sun, 06 Sep 2020 14:35:21 +0200
> Cc: 43218@debbugs.gnu.org, monnier@iro.umontreal.ca
> 
> It'd be nice if Emacs had a real drop-down in-buffer menu -- that's how
> other browsers present a select box.  Or has Emacs gotten that lately?

We have Company.

But why not simply a drop-down menu, a real one?

bug#43218: EWW handles default answer incorrectly when changing a select

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> But why not simply a drop-down menu, a real one?

That'd be great.  Do we have those?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43218: EWW handles default answer incorrectly when changing a select

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: nicolas.graner@universite-paris-saclay.fr,  43218@debbugs.gnu.org,
>   monnier@iro.umontreal.ca
> Date: Sun, 06 Sep 2020 16:41:42 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > But why not simply a drop-down menu, a real one?
> 
> That'd be great.  Do we have those?

Yes, sure.  See popup-menu and x-popup-menu.

bug#43218: EWW handles default answer incorrectly when changing a select

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> That'd be great.  Do we have those?
>
> Yes, sure.  See popup-menu and x-popup-menu.

Nice; never used popup-menu before (I think).

I've now changed eww to use menus for selects (and also made it possible
to use the mouse to click on a select value).  Looks pretty good, I
think...

I think this fixes all the problems discussed in this bug report.  The
<select multiple> is not handled, but that's a separate report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43218: EWW handles default answer incorrectly when changing a select

[Drew Adams]

> > Not sure how the prompting gets involved in this problem: if you decide
> > to let the user choose only based on the text (i.e. "one" vs "one"),
> > then there's not much the prompting can do to help: the minibuffer lets
> > the users enter a string and that's that.
> > If they type "one" which one of the two should you pick?
> 
> Yeah, that makes no sense.  I was kinda sorta thinking about how ido
> presents choices, but that's pretty much irrelevant in this case.

No, it does make sense.  Or rather, it can make sense.

Obviously, if all a user has to go on is the display
name of candidates, then she can't tell one from the
other.

But if other information, in addition to the display
name, is communicated in some way, either by default
or on demand, then it can make perfect sense.

The point is that the `completing-read' behavior,
which shows only the car of an alist entry (plus
possibly an annotation), is quite limited.

Examples of showing additional info:

* In Icicles search, there may be multiple search
  hits with the same text, at different locations.
  (1) The order in *Completions* can tell you where
  they are, relatively.  (2) You can hit a key to
  go to any search hit, to see exactly where it is,
  without that location being your final destination
  choice.

* In Bookmark+, you can have "autofile" bookmarks,
  whose names are the nondir destination file names.
  That's convenient.  But in the bookmark display
  list, or when jumping to a bookmark (completion)
  you can see what the destination file is.

There are other examples, with different behaviors
and use cases.

The point is that the car of an alist entry (which
is a string) does not, in general, carry all of the
information about the alist entry.  And that string
is all that `completing-read' shows (plus possibly
an annotation), and it is all that can be completed
against or otherwise used to act on.

There are ways for users to get the convenience of
seeing only those display strings, but at the same
time being able to identify/distinguish candidates
that have the same display string but have other
associated information (part of the full candidate).

It is false to think that both the user and
completion need necessarily be blind to such full
information, i.e., such differences.  After all,
`completing-read' knows the full candidate.  In
vanilla Emacs it just simply ignores it for its
job of completing.  That info is essentially
thrown away.

(Note that this is another case where an alist
is richer than a hash table or an obarray, which
exclude duplicate keys.)
___

A related question is whether `completing-read'
should remove duplicate candidates (where the
notion of "same" compares the display strings).
Obviously, if duplicates are removed then you
never see more than one `foo' candidate, and you
can't take advantage of the kinds of things I've
mentioned.

The answer to that "whether" question is that
it depends on the reason for completion and the
particular completion behavior required by the
command that calls `completing-read'.

In Icicles, variable `icicle-transform-function'
is a function that transforms the set of
completion candidates before they're displayed.
By default, no transformation is done, but you
can toggle this anytime during completion with
`C-$'.

When active, the default transformation is to
remove duplicates.  So by default, `C-$'
toggles removal of duplicate candidates.

bug#43218: EWW handles default answer incorrectly when changing a select

[Lars Ingebrigtsen]

Drew Adams <drew.adams@oracle.com> writes:

> The point is that the `completing-read' behavior,
> which shows only the car of an alist entry (plus
> possibly an annotation), is quite limited.

Yeah.  And I now remember why the question seemed so familiar to me -- I
think I asked the same question a couple a years ago when I wrote a mode
for doing IMDB searches, and I ended up with (I see now; I'd forgotten
all about this):

(defun imdb-completing-read (prompt collection)
  (let ((completion-in-region-function
	 (lambda (start end _ &optional __)
	   (let ((string (buffer-substring start end)))
	     (when (> (length string) 2)
	       (imdb-complete string collection)))))
	(minibuffer-allow-text-properties t))
    (read-from-minibuffer prompt nil imdb-minibuffer-local-completion-map)))

But I guess I still wonder why `completing-read' strips the text
properties from the completions?  If it's historical reasons, why not
allow minibuffer-allow-text-properties to override that?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43218: EWW handles default answer incorrectly when changing a select

[Drew Adams]

> > The point is that the `completing-read' behavior,
> > which shows only the car of an alist entry (plus
> > possibly an annotation), is quite limited.
> 
> Yeah.  And I now remember why the question seemed so familiar to me -- I
> think I asked the same question a couple a years ago when I wrote a mode
> for doing IMDB searches, and I ended up with (I see now; I'd forgotten
> all about this):...
> 
> But I guess I still wonder why `completing-read' strips the text
> properties from the completions?  If it's historical reasons, why
> not allow minibuffer-allow-text-properties to override that?

`completing-read' is likely older than text properties
on strings.  (It was coded only in C for a long time.)

As I said, `minibuffer-allow-text-properties' only has
an effect on text that is in the minibuffer.  When you
complete against candidates, the result may or may not
ever get put into the minibuffer, and it has no text
properties.

bug#43218: EWW handles default answer incorrectly when changing a select

[Lars Ingebrigtsen]

Drew Adams <drew.adams@oracle.com> writes:

>> But I guess I still wonder why `completing-read' strips the text
>> properties from the completions?  If it's historical reasons, why
>> not allow minibuffer-allow-text-properties to override that?
>
> `completing-read' is likely older than text properties
> on strings.  (It was coded only in C for a long time.)
>
> As I said, `minibuffer-allow-text-properties' only has
> an effect on text that is in the minibuffer.  When you
> complete against candidates, the result may or may not
> ever get put into the minibuffer, and it has no text
> properties.

Right.  What I'm wondering is whether we could make the text properties
in the candidates survive the completion process is that variable is
non-nil.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43218: EWW handles default answer incorrectly when changing a select

[Nicolas Graner]

Lars Ingebrigtsen <larsi@gnus.org> wrote on 06/09/2020 at 19h05 :
> I've now changed eww to use menus for selects (and also made it possible
> to use the mouse to click on a select value).  Looks pretty good, I
> think...

It does.

> I think this fixes all the problems discussed in this bug report.  The
> <select multiple> is not handled, but that's a separate report.

Before I file new bug reports :) you might want to consider:

* making 'disabled' options unselectable.

* making the current value the default option (i.e. typing <RET> right
  away should leave the value unchanged rather than select the first
  option). This is admittedly a feature request rather than a bug report.

For the record, the popup-menu does not seem accessible through the
speechd-el emacs screen reader, which makes eww less blind-friendly. But
that's an issue for the speechd-el developers.

Nicolas

bug#43218: EWW handles default answer incorrectly when changing a select

[Stefan Monnier]

> Right.  What I'm wondering is whether we could make the text properties
> in the candidates survive the completion process is that variable is
> non-nil.

While there might be some text-properties that should be stripped
(e.g. the `mouse-face` when copying text from *Completions* to the
minibuffer), I don't think there's any particular need to remove the
text-properties, no.  It probably gets removed as a side effect of
something else (e.g. of going through `try-completion`) so it probably
"just" needs more work to actively *preserve* the text-properties.


        Stefan

bug#43218: EWW handles default answer incorrectly when changing a select

[Lars Ingebrigtsen]

Nicolas Graner <nicolas.graner@universite-paris-saclay.fr> writes:

> Before I file new bug reports :) you might want to consider:
>
> * making 'disabled' options unselectable.
>
> * making the current value the default option (i.e. typing <RET> right
>   away should leave the value unchanged rather than select the first
>   option). This is admittedly a feature request rather than a bug report.

Sure, go ahead and file new bug report(s)...  this one is getting kinda
long.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43218: EWW handles default answer incorrectly when changing a select

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> It probably gets removed as a side effect of
> something else (e.g. of going through `try-completion`) so it probably
> "just" needs more work to actively *preserve* the text-properties.

FOUND IT!!!1!

Man, the completion stuff nests deep...

Anyway, the properties are removed here:

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 864726e3cc..2b188d3e95 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -1068,7 +1068,7 @@ completion--replace
   ;; The properties on `newtext' include things like
   ;; completions-first-difference, which we don't want to include
   ;; upon insertion.
-  (set-text-properties 0 (length newtext) nil newtext)
+  ;;(set-text-properties 0 (length newtext) nil newtext)
   ;; Maybe this should be in subr.el.
   ;; You'd think this is trivial to do, but details matter if you want
   ;; to keep markers "at the right place" and be robust in the face of

To it sounds like it's just nixing out all the text properties because
it wants to get rid of completions-first-difference and... other things?
If we could identify those other things, then we could remove just them
if minibuffer-allow-text-properties is set.

I've tried looking at what comes out of the machinery with this "patch",
and ... I'm not finding anything much.

Does this sound familiar to anybody?  I can grep around to see if
there's any more, I guess...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#43218: EWW handles default answer incorrectly when changing a select

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Does this sound familiar to anybody?  I can grep around to see if
> there's any more, I guess...

Well, there's a bunch of faces and at least one other property:

           (while md
             (funcall update-score start (car md))
             (add-face-text-property
              start (pop md)
              'completions-common-part
              nil str)
             (setq start (pop md)))
           (funcall update-score len len)
           (add-face-text-property
            start end
            'completions-common-part
            nil str)
           (if (> (length str) pos)
               (add-face-text-property
                pos (1+ pos)
                'completions-first-difference
                nil str))
           (unless (zerop (length str))
             (put-text-property
              0 1 'completion-score
              (/ score-numerator (* len (1+ score-denominator)) 1.0) str)))

We actually don't seem to have a remove-face-text-property?  But perhaps
is fine nixing out the face property as long as the other properties
survive.

(completion-score should be removed too, of course.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no